From 74cd3015aeb516c0d46312ef52ca2f65eace6de1 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 24 Jun 2026 10:07:51 -0500 Subject: [PATCH 1/2] PDX-516: feat(mcp): flag UiAssert/post-save steps under wrong screen RCA: Create-and-verify tests that leave the UiAssert nested under the action=New create screen scored a clean 100 with no warning, because no validator checked whether a verification step sits in the correct post-save screen context. Fix: Added UI-SCREEN-CONTEXT-001 (check.type uiAssertScreenContext, MULTI_VALIDATOR_REGISTRY) with two heuristics over existing XML: UiAssert/UiRead nested under a UiWithScreen whose target URI carries action=New, and any UI action following a Save (locator binding action%3Dsave) within the same substeps block. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/VALIDATION_RULE_REGISTRY.md | 3 +- docs/mcp.md | 1 + .../rules/provar_best_practices_rules.json | 16 + src/mcp/tools/bestPracticesEngine.ts | 150 ++++++++ .../opportunity-create-verify-bad.testcase | 271 +++++++++++++++ .../opportunity-create-verify-good.testcase | 324 ++++++++++++++++++ test/unit/mcp/bestPracticesEngine.test.ts | 159 +++++++++ 7 files changed, 923 insertions(+), 1 deletion(-) create mode 100644 test/fixtures/opportunity-create-verify-bad.testcase create mode 100644 test/fixtures/opportunity-create-verify-good.testcase diff --git a/docs/VALIDATION_RULE_REGISTRY.md b/docs/VALIDATION_RULE_REGISTRY.md index 2e32373..2269776 100644 --- a/docs/VALIDATION_RULE_REGISTRY.md +++ b/docs/VALIDATION_RULE_REGISTRY.md @@ -11,7 +11,7 @@ Provar test-case validation runs in two layers. This registry is the single cano **The validity bridge (PDX-509):** a `critical` best-practice violation is surfaced into `issues[]` as an `ERROR` and therefore gates `is_valid` — EXCEPT where a Layer-1 check already owns the concept (then it is suppressed to avoid double-reporting). `major`/`minor`/`info` affect `quality_score` (and the `needs_improvement` status) only. The `status` field is tri-state: `invalid` (a critical) / `needs_improvement` (loads but `quality_score < quality_threshold`) / `valid`. -**Counts:** Layer 1 — 23 rules (18 gating). Layer 2 — 178 rules (critical 64 / major 67 / minor 29 / info 18; 58 bridged to `is_valid`). +**Counts:** Layer 1 — 23 rules (18 gating). Layer 2 — 179 rules (critical 64 / major 68 / minor 29 / info 18; 58 bridged to `is_valid`). ## Layer 1 — Structural validity rules @@ -194,6 +194,7 @@ Provar test-case validation runs in two layers. This registry is the single cano | `UI-LOCATOR-SAVE-001` | TestCaseDesign | major | 5 | No | Save button locator must use correct pattern. | | `UI-LOOKUP-ID-001` | TestCaseDesign | major | 6 | No | UiDoAction lookup fields should use Name values, not IDs. | | `UI-NAVIGATE-PREFER-SCREEN-001` | TestCaseDesign | info | 1 | No | Prefer UiWithScreen over UiNavigate for Salesforce. | +| `UI-SCREEN-CONTEXT-001` | TestCaseDesign | major | 5 | No | UI verification or post-save step left under the wrong screen context. | | `UI-SCREEN-NAV-001` | TestCaseDesign | major | 5 | No | First UiWithScreen must use navigate=Always or IfNeccessary. | | `UI-SCREEN-NAV-002` | TestCaseDesign | minor | 2 | No | First UiWithScreen should prefer navigate=Always over IfNeccessary. | | `UI-SCREEN-OBJID-001` | TestCaseDesign | major | 5 | No | UiWithScreen with navigate=Always for Edit/View must have sfUiTargetObjectId. | diff --git a/docs/mcp.md b/docs/mcp.md index bf7b87a..a530ddc 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1101,6 +1101,7 @@ Validates an XML test case for schema correctness (validity score) and best prac - **`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. + - **`UI-SCREEN-CONTEXT-001`** (`uiAssertScreenContext`) — a `UiAssert`/`UiRead` left nested under a create screen (`UiWithScreen` whose target URI carries `action=New`), or any UI action placed **after a Save** (`UiDoAction` whose locator binding contains `action=save`, percent-encoded as `action%3Dsave`) inside the same `UiWithScreen` `` block. Salesforce tears down the create form on Save and navigates to the saved record's View screen, so a verification still under `action=New` asserts against a page that no longer exists, and steps following a Save in the same wrapper run in stale context. The test still loads and may score 100, so this is a runtime/logic correctness issue. Fix: close the create screen at the Save step and open a new `UiWithScreen` targeting `action=View` (capturing the new record id via `sfUiTargetResultName`) for the verification or follow-up action. Emits one violation per offending step. **Coverage limit:** this is a structural heuristic — a passing run confirms the create/verify split is structured correctly, not that the assertion logic itself is correct. - **Structural correctness rules** — Checks on element shape / required structure that the Provar IDE depends on to render and run a step. Mostly critical; like the rules above they run offline / in `local_fallback` and keep score parity with the Quality Hub back-end. Currently enforced: - **`SETVALUES-STRUCTURE-001`** (`setValuesStructure`, critical) — a `SetValues` step has no `` container. - **`SETVALUES-NAME-001`** (`namedValueName`, critical) — a `` inside a `SetValues` step is missing its `name` attribute. diff --git a/src/mcp/rules/provar_best_practices_rules.json b/src/mcp/rules/provar_best_practices_rules.json index 8ba7b00..5058ac9 100644 --- a/src/mcp/rules/provar_best_practices_rules.json +++ b/src/mcp/rules/provar_best_practices_rules.json @@ -2959,6 +2959,22 @@ "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": "UI-SCREEN-CONTEXT-001", + "category": "TestCaseDesign", + "name": "UI verification or post-save step left under the wrong screen context", + "description": "A UiAssert or UiRead nested under a create screen (UiWithScreen target action=New), or any UI action placed after a Save (action=save) inside the same UiWithScreen substeps block, is almost certainly bound to the wrong screen. Salesforce tears down the create form on Save and navigates to the saved record's View screen, so a verification still nested under action=New asserts against a page that no longer exists, and steps following a Save in the same wrapper run in stale context. The test case still loads and may even score 100, so this is a runtime/logic correctness issue (major), not a load failure.", + "appliesTo": ["Step", "TestCase"], + "severity": "major", + "weight": 5, + "recommendation": "Close the create UiWithScreen at the Save step and open a new UiWithScreen targeting the post-save screen for any verification or follow-up action. For Salesforce record creation, assert the saved record under a UiWithScreen with target sf:ui:target?object=&action=View&recordId={}, capturing the created record id via sfUiTargetResultName on the create screen. Do not place UiAssert/UiRead steps under an action=New screen, and do not leave steps after a Save inside the create screen's substeps block.", + "check": { + "type": "uiAssertScreenContext", + "target": "step" + }, + "notes": "Two heuristics over data already in the XML: (1) UiAssert/UiRead whose nearest enclosing UiWithScreen target URI carries action=New; (2) any UI action that appears after a Save UiDoAction (locator binding action%3Dsave, percent-encoded in the URI) within the same block. Emits one violation per offending step so (rule_id, test_item_id) de-dups cleanly. Coverage limit: this is a structural heuristic — a passing run confirms the create/verify split is structured correctly, not that the assertion logic itself is correct.", + "source": "Field observation: AI-generated create-and-verify tests leave the UiAssert under the action=New screen, scoring 100 while asserting against the wrong page at runtime" + }, { "id": "VAR-STRING-LITERAL-001", "category": "TestCaseDesign", diff --git a/src/mcp/tools/bestPracticesEngine.ts b/src/mcp/tools/bestPracticesEngine.ts index ffd49c9..70c3446 100644 --- a/src/mcp/tools/bestPracticesEngine.ts +++ b/src/mcp/tools/bestPracticesEngine.ts @@ -946,6 +946,155 @@ function validateUiActionNestingStructure(tc: XmlNode, rule: BPRule): BPViolatio return violations; } +// ── UI-SCREEN-CONTEXT-001 — assert/post-save steps under the wrong screen ───── +// A UiAssert / UiRead that verifies a persisted record while still nested under +// the `action=New` create screen is almost always wrong context: the create +// screen tears down after Save, so the assertion runs against the wrong page. +// Likewise any UI action that follows a Save (`action=save`) inside the SAME +// UiWithScreen substeps block is suspect — Save navigates away, so trailing +// steps belong in a fresh UiWithScreen (typically `action=View`). + +// UI action apiIds whose presence AFTER a Save (in the same screen block) is suspect. +const POST_SAVE_SUSPECT_APIS = UI_ACTION_API_IDS; +// Read-only verification steps that should not run on the create (`action=New`) screen. +const ASSERT_ON_NEW_SUSPECT_APIS = new Set([ + 'com.provar.plugins.forcedotcom.core.ui.UiAssert', + 'com.provar.plugins.forcedotcom.core.ui.UiRead', +]); +const UI_WITH_SCREEN_API_ID = 'com.provar.plugins.forcedotcom.core.ui.UiWithScreen'; + +/** + * Read the URI string carried by a UiWithScreen `target` argument. The value is + * stored either as the `uri` attribute (``) or, + * for hand-authored XML, as element text. Returns undefined when absent. + */ +function getUiWithScreenTargetUri(call: XmlNode): string | undefined { + for (const arg of getCallArguments(call)) { + if (arg['@_id'] !== 'target') continue; + const valElem = arg['value'] as XmlNode | undefined; + if (!valElem || typeof valElem !== 'object') return undefined; + const uriAttr = valElem['@_uri'] as string | undefined; + if (uriAttr) return uriAttr; + return (valElem['#text'] as string | undefined) ?? undefined; + } + return undefined; +} + +/** Parse the `action` query parameter from a UiWithScreen target URI (e.g. `…?object=Opportunity&action=New`). */ +function getScreenAction(targetUri: string | undefined): string | undefined { + if (!targetUri || !targetUri.includes('?')) return undefined; + const qs = targetUri.slice(targetUri.indexOf('?') + 1); + return new URLSearchParams(qs).get('action') ?? undefined; +} + +/** Read the `uri` attribute of a named argument's `` element (e.g. locator / interaction). */ +function getArgUri(call: XmlNode, argId: string): string | undefined { + for (const arg of getCallArguments(call)) { + if (arg['@_id'] !== argId) continue; + const valElem = arg['value'] as XmlNode | undefined; + if (!valElem || typeof valElem !== 'object') return undefined; + return (valElem['@_uri'] as string | undefined) ?? undefined; + } + return undefined; +} + +/** + * True when a UiDoAction is a Save click. The locator binding encodes the action + * percent-encoded (`action%3Dsave`); some authoring paths instead use a `name=save` + * locator with an `action` interaction. Either signature counts as a Save. + */ +function isSaveAction(call: XmlNode): boolean { + if (call['@_apiId'] !== 'com.provar.plugins.forcedotcom.core.ui.UiDoAction') return false; + const locator = getArgUri(call, 'locator')?.toLowerCase() ?? ''; + if (locator.includes('action%3dsave') || locator.includes('action=save')) return true; + const interaction = getArgUri(call, 'interaction')?.toLowerCase() ?? ''; + return /[?&]name=save(&|$)/.test(locator) && interaction.includes('name=action'); +} + +/** The direct steps inside a UiWithScreen's `` block (in document order). */ +function getScreenSubstepCalls(screen: XmlNode): XmlNode[] { + const clauses = screen['clauses'] as XmlNode | undefined; + if (!clauses || typeof clauses !== 'object') return []; + for (const clause of toArr(clauses['clause'] as XmlNode | XmlNode[])) { + if (!clause || typeof clause !== 'object') continue; + if (clause['@_name'] !== 'substeps') continue; + const steps = clause['steps'] as XmlNode | undefined; + if (!steps || typeof steps !== 'object') continue; + return toArr(steps['apiCall'] as XmlNode | XmlNode[]).filter((c) => c && typeof c === 'object'); + } + return []; +} + +/** Human-readable label for an offending step (`'Title' (testItemId=N)`). */ +function describeStep(call: XmlNode): string { + const ctx = stepContext(call); + return `${ctx.apiName} '${ctx.title}' (testItemId=${ctx.tid})`; +} + +/** + * Heuristic 1 — a UiAssert/UiRead whose nearest enclosing UiWithScreen targets the + * create screen (`action=New`). Emits one violation per offending verification step. + */ +function flagAssertsOnNewScreen(screen: XmlNode, rule: BPRule, out: BPViolation[]): void { + const action = (getScreenAction(getUiWithScreenTargetUri(screen)) ?? '').toLowerCase(); + if (action !== 'new') return; + for (const step of getScreenSubstepCalls(screen)) { + const apiId = step['@_apiId'] as string | undefined; + if (!apiId || !ASSERT_ON_NEW_SUSPECT_APIS.has(apiId)) continue; + out.push( + makeViolation( + rule, + `${describeStep(step)} verifies a persisted record while nested under the create screen ` + + '(action=New). The create screen is torn down after Save, so the assertion runs against the ' + + 'wrong page context. Move it into a new UiWithScreen targeting action=View (or the appropriate post-save screen).' + ) + ); + } +} + +/** + * Heuristic 2 — any UI action step that appears AFTER a Save click inside the SAME + * UiWithScreen substeps block. Save navigates away, so trailing steps are suspect. + * Emits one violation per offending trailing step. + */ +function flagStepsAfterSave(screen: XmlNode, rule: BPRule, out: BPViolation[]): void { + const calls = getScreenSubstepCalls(screen); + let sawSave = false; + for (const step of calls) { + if (!sawSave) { + if (isSaveAction(step)) sawSave = true; + continue; + } + const apiId = step['@_apiId'] as string | undefined; + if (!apiId || !POST_SAVE_SUSPECT_APIS.has(apiId)) continue; + out.push( + makeViolation( + rule, + `${describeStep(step)} appears after a Save (action=save) inside the same UiWithScreen block. ` + + 'Save navigates away from the screen, so this step runs in stale context. Move it into a new ' + + 'UiWithScreen targeting action=View (or the appropriate post-save screen).' + ) + ); + } +} + +/** + * UI-SCREEN-CONTEXT-001 — flag UiAssert/UiRead steps left under the `action=New` + * create screen, and any UI action that follows a Save within the same screen + * block. Both heuristics read data already present in the XML (target URIs and + * locator bindings) and emit ONE violation per offending step so that + * (rule_id, test_item_id) de-dups cleanly. + */ +function validateUiAssertScreenContext(tc: XmlNode, rule: BPRule): BPViolation[] { + const violations: BPViolation[] = []; + for (const call of getAllApiCalls(tc)) { + if (call['@_apiId'] !== UI_WITH_SCREEN_API_ID) continue; + flagAssertsOnNewScreen(call, rule, violations); + flagStepsAfterSave(call, rule, violations); + } + return violations; +} + // ── NitroX MS variant validators (UI-NITROX-CONNECT-ARGS-001, UI-NITROX-VARIANT-ARG-001) ─── /** @@ -2200,6 +2349,7 @@ type MultiValidatorFn = (tc: XmlNode, rule: BPRule) => BPViolation[]; const MULTI_VALIDATOR_REGISTRY: Record = { uiActionNestingStructure: validateUiActionNestingStructure, + uiAssertScreenContext: validateUiAssertScreenContext, nitroxConnectInvalidArgs: validateNitroxConnectInvalidArgs, nitroxVariantArgRequired: validateNitroxVariantArgRequired, // Tier 4 — emits one violation per offending value (back-end returns a list) diff --git a/test/fixtures/opportunity-create-verify-bad.testcase b/test/fixtures/opportunity-create-verify-bad.testcase new file mode 100644 index 0000000..55e9ba2 --- /dev/null +++ b/test/fixtures/opportunity-create-verify-bad.testcase @@ -0,0 +1,271 @@ + + + Log into Salesforce as a Sales Representative, create a new Opportunity populating all mandatory fields (Opportunity Name, Close Date, Stage), save it, and verify the Opportunity is created successfully. Precondition: SalesRepConnection is configured. Outcome: a new Opportunity named 'Provar Test Opportunity' exists and its Name is asserted on the saved record. NOTE: this fixture deliberately leaves the UiAssert nested under the action=New create screen, after the Save, to exercise UI-SCREEN-CONTEXT-001. + + + + + SalesRepConnection + + + SalesRepConnection + + + Test + + + LightningSales + + + true + + + true + + + default + + + Fail + + + true + + + true + + + 0339c9e0-a309-44ed-ba42-6525553f9001 + + + + + + + + + + + + + + + + SalesRepConnection + + + + + + Always + + + On SF Opportunity Home screen + + + Default + + + Default + + + false + + + false + + + + + + + + + + + + + + + Click the New button + + + Default + + + false + + + false + + + + + + + + + + + SalesRepConnection + + + + + + Dont + + + On SF Opportunity New screen + + + opportunityId + + + Test + + + Default + + + Default + + + false + + + false + + + + + + + + + + + + + + + Provar Test Opportunity + + + Set the Opportunity Name field to Provar Test Opportunity + + + false + + + false + + + + + + + + + + + + + 12/31/2026 + + + Set the Close Date field to 12/31/2026 + + + false + + + false + + + + + + + + + + + + + Prospecting + + + Set the Stage field to Prospecting + + + false + + + false + + + + + + + + + + + + + Click the Save button + + + full + + + Default + + + + + + + + + + OpportunityNameResult + + + Test + + + + + + + + Provar Test Opportunity + + + + + + + + + + + + + false + + + + + + + + + + + + + diff --git a/test/fixtures/opportunity-create-verify-good.testcase b/test/fixtures/opportunity-create-verify-good.testcase new file mode 100644 index 0000000..44fb8ae --- /dev/null +++ b/test/fixtures/opportunity-create-verify-good.testcase @@ -0,0 +1,324 @@ + + + Log into Salesforce as a Sales Representative, create a new Opportunity populating all mandatory fields (Opportunity Name, Close Date, Stage), save it, and verify the Opportunity is created successfully. Precondition: SalesRepConnection is configured. Outcome: a new Opportunity named 'Provar Test Opportunity' exists and its Name is asserted on the saved record. + + + + + SalesRepConnection + + + SalesRepConnection + + + Test + + + LightningSales + + + true + + + true + + + default + + + Fail + + + true + + + true + + + 0339c9e0-a309-44ed-ba42-6525553f9001 + + + + + + + + + + + + + + + + SalesRepConnection + + + + + + Always + + + On SF Opportunity Home screen + + + Default + + + Default + + + false + + + false + + + + + + + + + + + + + + + Click the New button + + + Default + + + false + + + false + + + + + Whether the mouse should hover over the field before it is clicked. + + + + + + How the click should be executed for Internet Explorer. + + + + com.provar.core.model.base.java.EnumChoiceListValuesSource + com.provar.core.model.ui.api.ClickMethod + + + + + + + + + + + + + + SalesRepConnection + + + + + + Dont + + + On SF Opportunity New screen + + + opportunityId + + + Test + + + Default + + + Default + + + false + + + false + + + + + + + + + + + + + + + Provar Test Opportunity + + + Set the Opportunity Name field to Provar Test Opportunity + + + false + + + false + + + + + + + + + + + + + 12/31/2026 + + + Set the Close Date field to 12/31/2026 + + + false + + + false + + + + + + + + + + + + + Prospecting + + + Set the Stage field to Prospecting + + + false + + + false + + + + + + + + + + + + + Click the Save button + + + full + + + Default + + + + + + + + + + + SalesRepConnection + + + + + + Dont + + + On SF Opportunity View screen + + + Default + + + Default + + + false + + + false + + + + + + + + + + + + OpportunityNameResult + + + Test + + + + + + + + Provar Test Opportunity + + + + + + + + + + + + + false + + + + + + + + + + + + + diff --git a/test/unit/mcp/bestPracticesEngine.test.ts b/test/unit/mcp/bestPracticesEngine.test.ts index 3b501fc..ffbcb97 100644 --- a/test/unit/mcp/bestPracticesEngine.test.ts +++ b/test/unit/mcp/bestPracticesEngine.test.ts @@ -420,6 +420,165 @@ describe('runBestPractices', () => { } }); + // ── UI-SCREEN-CONTEXT-001 — assert / post-save under the wrong screen ── + describe('UI-SCREEN-CONTEXT-001 — uiAssertScreenContext validator', () => { + const GUID_TCSC = '550e8400-e29b-41d4-a716-446655440050'; + const GUID_UWSSC = '550e8400-e29b-41d4-a716-446655440051'; + const GUID_DOSC = '550e8400-e29b-41d4-a716-446655440052'; + const GUID_XSC = '550e8400-e29b-41d4-a716-446655440053'; + + function screenViolations(violations: BPViolation[]): BPViolation[] { + return violations.filter((v) => v.rule_id === 'UI-SCREEN-CONTEXT-001'); + } + + async function readFixture(name: string): Promise { + const fs = await import('node:fs'); + const path = await import('node:path'); + const url = await import('node:url'); + const here = path.dirname(url.fileURLToPath(import.meta.url)); + return fs.readFileSync(path.join(here, '..', '..', 'fixtures', name), 'utf-8'); + } + + // ─ POSITIVE: constructed bad fixture (assert under action=New, after Save) fires ─ + it('fires on the constructed bad fixture (UiAssert under action=New, after Save)', async () => { + const xml = await readFixture('opportunity-create-verify-bad.testcase'); + const result = runBestPractices(xml); + const vs = screenViolations(result.violations); + assert.ok(vs.length >= 1, `Expected UI-SCREEN-CONTEXT-001 to fire; got ${vs.length}`); + assert.equal(vs[0].severity, 'major'); + assert.equal(vs[0].weight, 5); + assert.ok( + vs.some((v) => v.message.includes('testItemId=14')), + `Expected a violation naming the offending UiAssert (testItemId=14): ${JSON.stringify( + vs.map((v) => v.message) + )}` + ); + assert.ok(result.quality_score < 100, `Expected quality_score < 100, got ${result.quality_score}`); + }); + + // ─ NEGATIVE: good fixture (assert under action=View) does NOT fire ─ + it('does not fire on the good fixture (UiAssert under action=View)', async () => { + const xml = await readFixture('opportunity-create-verify-good.testcase'); + const vs = screenViolations(runBestPractices(xml).violations); + assert.equal(vs.length, 0, `Expected no UI-SCREEN-CONTEXT-001; got: ${JSON.stringify(vs.map((v) => v.message))}`); + }); + + // ─ Heuristic 1: UiAssert directly under an action=New screen fires ─ + it('fires for a UiAssert nested under an action=New screen', () => { + const xml = ` + + + + + + + + + + + + + + + + + +`; + const vs = screenViolations(runBestPractices(xml).violations); + assert.equal(vs.length, 1, `Expected one violation; got ${JSON.stringify(vs.map((v) => v.message))}`); + assert.ok(vs[0].message.includes('action=New'), `Message should mention action=New: ${vs[0].message}`); + assert.ok(vs[0].message.includes('testItemId=3'), `Message should name the step: ${vs[0].message}`); + }); + + // ─ Heuristic 2: any UI action after a Save in the same screen block fires ─ + it('fires for a step that follows a Save within the same UiWithScreen block', () => { + const xml = ` + + + + + + + + + + + + + + + + + + + + + + + + + + + +`; + const vs = screenViolations(runBestPractices(xml).violations); + assert.ok( + vs.some((v) => v.message.includes('testItemId=4') && v.message.includes('after a Save')), + `Expected a post-save violation for the trailing step (testItemId=4): ${JSON.stringify( + vs.map((v) => v.message) + )}` + ); + }); + + // ─ Clean minimal: assert under action=View, nothing after Save → no violation ─ + it('does not fire when the assert is under an action=View screen and nothing follows Save', () => { + const xml = ` + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +`; + const vs = screenViolations(runBestPractices(xml).violations); + assert.equal(vs.length, 0, `Expected no UI-SCREEN-CONTEXT-001; got: ${JSON.stringify(vs.map((v) => v.message))}`); + }); + }); + // ── UI-NITROX-CONNECT-ARGS-001 / UI-NITROX-VARIANT-ARG-001 — NitroX MS variants ── describe('NitroX MS variants (Dynamics 365 + Power Platform)', () => { From f8a3b7d367363c66a51fdd885eae520dde02acb0 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 24 Jun 2026 10:58:24 -0500 Subject: [PATCH 2/2] PDX-516: fix(mcp): scope UiAssert screen-context rule and de-dup score RCA: The first cut of UI-SCREEN-CONTEXT-001 matched Save via raw substring (action%3dsave) so Save & New false-matched, ran heuristic 2 on every UiWithScreen so post-save steps under Edit/View false-fired, and emitted two violations for one step (assert after Save under New) double-penalising the local score (92.5 not 96.25). Fix: isSaveAction now parses the locator binding and compares the decoded action token exactly to save; heuristic 2 is scoped to action=New only; validateUiAssertScreenContext de-dups to at most one violation per offending testItemId. Added Edit and Save & New false-positive guard tests, locked the bad fixture to one violation and score 96.25, refreshed docs/notes, and bumped version 1.6.2 to 1.6.3. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/mcp.md | 2 +- package.json | 2 +- server.json | 4 +- .../rules/provar_best_practices_rules.json | 4 +- src/mcp/tools/bestPracticesEngine.ts | 94 ++++++++++++---- test/unit/mcp/bestPracticesEngine.test.ts | 101 ++++++++++++++++-- 6 files changed, 171 insertions(+), 36 deletions(-) diff --git a/docs/mcp.md b/docs/mcp.md index a530ddc..80e6030 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1101,7 +1101,7 @@ Validates an XML test case for schema correctness (validity score) and best prac - **`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. - - **`UI-SCREEN-CONTEXT-001`** (`uiAssertScreenContext`) — a `UiAssert`/`UiRead` left nested under a create screen (`UiWithScreen` whose target URI carries `action=New`), or any UI action placed **after a Save** (`UiDoAction` whose locator binding contains `action=save`, percent-encoded as `action%3Dsave`) inside the same `UiWithScreen` `` block. Salesforce tears down the create form on Save and navigates to the saved record's View screen, so a verification still under `action=New` asserts against a page that no longer exists, and steps following a Save in the same wrapper run in stale context. The test still loads and may score 100, so this is a runtime/logic correctness issue. Fix: close the create screen at the Save step and open a new `UiWithScreen` targeting `action=View` (capturing the new record id via `sfUiTargetResultName`) for the verification or follow-up action. Emits one violation per offending step. **Coverage limit:** this is a structural heuristic — a passing run confirms the create/verify split is structured correctly, not that the assertion logic itself is correct. + - **`UI-SCREEN-CONTEXT-001`** (`uiAssertScreenContext`) — a `UiAssert`/`UiRead` left nested under a create screen (`UiWithScreen` whose target URI carries `action=New`), or any UI action placed **after a plain Save** inside the same `action=New` `` block. Save is detected by parsing the `UiDoAction` locator binding's decoded `action` token and matching it **exactly** to `save` (case-insensitive), so continuation buttons like **Save & New** (`action=saveAndNew`) are not treated as a Save. Salesforce tears down the create form on Save and navigates to the saved record's View screen, so a verification still under `action=New` asserts against a page that no longer exists, and steps following the Save in the same create wrapper run in stale context. **Both checks are scoped to `action=New`** — post-save steps/asserts under `action=Edit`/`View` are a valid same-record pattern and do **not** fire. The test still loads and may score 100, so this is a runtime/logic correctness issue. Fix: close the create screen at the Save step and open a new `UiWithScreen` targeting `action=View` (capturing the new record id via `sfUiTargetResultName`) for the verification or follow-up action. A step that trips both checks (assert placed after Save under New) is **de-duped to one violation per offending step** inside the local engine before scoring, so it is penalised once (major × weight 5 → score 96.25), not twice — this matters because the local engine sums every violation and has no `test_item_id` field of its own. **Coverage limit:** this is a structural heuristic — a passing run confirms the create/verify split is structured correctly, not that the assertion logic itself is correct. - **Structural correctness rules** — Checks on element shape / required structure that the Provar IDE depends on to render and run a step. Mostly critical; like the rules above they run offline / in `local_fallback` and keep score parity with the Quality Hub back-end. Currently enforced: - **`SETVALUES-STRUCTURE-001`** (`setValuesStructure`, critical) — a `SetValues` step has no `` container. - **`SETVALUES-NAME-001`** (`namedValueName`, critical) — a `` inside a `SetValues` step is missing its `name` attribute. diff --git a/package.json b/package.json index cd13164..96cb040 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.6.2", + "version": "1.6.3", "mcpName": "io.github.ProvarTesting/provar", "license": "BSD-3-Clause", "plugins": [ diff --git a/server.json b/server.json index 9b57a70..c32307e 100644 --- a/server.json +++ b/server.json @@ -14,12 +14,12 @@ "url": "https://github.com/ProvarTesting/provardx-cli", "source": "github" }, - "version": "1.6.2", + "version": "1.6.3", "packages": [ { "registryType": "npm", "identifier": "@provartesting/provardx-cli", - "version": "1.6.2", + "version": "1.6.3", "transport": { "type": "stdio" }, diff --git a/src/mcp/rules/provar_best_practices_rules.json b/src/mcp/rules/provar_best_practices_rules.json index 5058ac9..80248df 100644 --- a/src/mcp/rules/provar_best_practices_rules.json +++ b/src/mcp/rules/provar_best_practices_rules.json @@ -2963,7 +2963,7 @@ "id": "UI-SCREEN-CONTEXT-001", "category": "TestCaseDesign", "name": "UI verification or post-save step left under the wrong screen context", - "description": "A UiAssert or UiRead nested under a create screen (UiWithScreen target action=New), or any UI action placed after a Save (action=save) inside the same UiWithScreen substeps block, is almost certainly bound to the wrong screen. Salesforce tears down the create form on Save and navigates to the saved record's View screen, so a verification still nested under action=New asserts against a page that no longer exists, and steps following a Save in the same wrapper run in stale context. The test case still loads and may even score 100, so this is a runtime/logic correctness issue (major), not a load failure.", + "description": "A UiAssert or UiRead nested under a create screen (UiWithScreen target action=New), or any UI action placed after a plain Save (action=save) inside the same action=New substeps block, is almost certainly bound to the wrong screen. Salesforce tears down the create form on Save and navigates to the saved record's View screen, so a verification still nested under action=New asserts against a page that no longer exists, and steps following the Save in the same create wrapper run in stale context. Both checks are scoped to action=New: post-save steps under action=Edit/View are a valid same-record pattern and do not fire, and continuation buttons like Save & New (action=saveAndNew) are not treated as a Save. The test case still loads and may even score 100, so this is a runtime/logic correctness issue (major), not a load failure.", "appliesTo": ["Step", "TestCase"], "severity": "major", "weight": 5, @@ -2972,7 +2972,7 @@ "type": "uiAssertScreenContext", "target": "step" }, - "notes": "Two heuristics over data already in the XML: (1) UiAssert/UiRead whose nearest enclosing UiWithScreen target URI carries action=New; (2) any UI action that appears after a Save UiDoAction (locator binding action%3Dsave, percent-encoded in the URI) within the same block. Emits one violation per offending step so (rule_id, test_item_id) de-dups cleanly. Coverage limit: this is a structural heuristic — a passing run confirms the create/verify split is structured correctly, not that the assertion logic itself is correct.", + "notes": "Two heuristics over data already in the XML, both scoped to UiWithScreen target action=New: (1) UiAssert/UiRead nested directly under the create screen; (2) any UI action that appears after a plain Save UiDoAction within the same block. Save is detected by parsing the locator binding's decoded action token and comparing it EXACTLY to 'save' (case-insensitive), so saveAndNew / savedSearch / savepoint never match. A step that trips both heuristics (assert placed after Save under New) is de-duped to a single violation per offending testItemId inside the local engine before scoring, so it is penalised once (major x weight 5); this matters because the local score sums every BPViolation and has no test_item_id field of its own. Coverage limit: this is a structural heuristic — a passing run confirms the create/verify split is structured correctly, not that the assertion logic itself is correct.", "source": "Field observation: AI-generated create-and-verify tests leave the UiAssert under the action=New screen, scoring 100 while asserting against the wrong page at runtime" }, { diff --git a/src/mcp/tools/bestPracticesEngine.ts b/src/mcp/tools/bestPracticesEngine.ts index 70c3446..f7f23d9 100644 --- a/src/mcp/tools/bestPracticesEngine.ts +++ b/src/mcp/tools/bestPracticesEngine.ts @@ -950,9 +950,11 @@ function validateUiActionNestingStructure(tc: XmlNode, rule: BPRule): BPViolatio // A UiAssert / UiRead that verifies a persisted record while still nested under // the `action=New` create screen is almost always wrong context: the create // screen tears down after Save, so the assertion runs against the wrong page. -// Likewise any UI action that follows a Save (`action=save`) inside the SAME -// UiWithScreen substeps block is suspect — Save navigates away, so trailing -// steps belong in a fresh UiWithScreen (typically `action=View`). +// Likewise any UI action that follows a plain Save (`action=save`, compared +// exactly so `Save & New` is excluded) inside the SAME `action=New` screen block +// is suspect — the create screen is torn down, so trailing steps belong in a +// fresh UiWithScreen (typically `action=View`). Both heuristics are scoped to +// `action=New`; post-save steps under `action=Edit`/`View` are a valid pattern. // UI action apiIds whose presence AFTER a Save (in the same screen block) is suspect. const POST_SAVE_SUSPECT_APIS = UI_ACTION_API_IDS; @@ -999,16 +1001,39 @@ function getArgUri(call: XmlNode, argId: string): string | undefined { } /** - * True when a UiDoAction is a Save click. The locator binding encodes the action - * percent-encoded (`action%3Dsave`); some authoring paths instead use a `name=save` - * locator with an `action` interaction. Either signature counts as a Save. + * Decode the `action` token a UiDoAction locator targets, lower-cased, or undefined. + * + * The locator URI looks like `ui:locator?name=save&binding=`, + * where the decoded binding is itself a query string (`sf:ui:binding:object?object=Opportunity&action=save`). + * We parse the outer locator for `binding`, decode it, and read its `action` param; + * we also accept the `name` param as a fallback when no binding action is present. + * Both are returned as EXACT tokens (no substring matching) so `saveAndNew`, + * `savedSearch`, `savepoint`, etc. never masquerade as a plain `save`. + */ +function getLocatorActionToken(call: XmlNode): string | undefined { + const locatorUri = getArgUri(call, 'locator'); + if (!locatorUri || !locatorUri.includes('?')) return undefined; + const locatorParams = new URLSearchParams(locatorUri.slice(locatorUri.indexOf('?') + 1)); + const binding = locatorParams.get('binding'); + if (binding && binding.includes('?')) { + // URLSearchParams already percent-decodes the binding value; parse its own query string. + const bindingAction = new URLSearchParams(binding.slice(binding.indexOf('?') + 1)).get('action'); + if (bindingAction) return bindingAction.toLowerCase(); + } + const nameToken = locatorParams.get('name'); + return nameToken ? nameToken.toLowerCase() : undefined; +} + +/** + * True when a UiDoAction is a plain Save click. The action token is compared + * EXACTLY (case-insensitive) against `save`, so continuation buttons such as + * `Save & New` (`action=saveAndNew` / `name=saveAndNew`) and inline-edit + * variants do NOT count — they keep the user on the create screen, so steps + * after them are legitimate. */ function isSaveAction(call: XmlNode): boolean { if (call['@_apiId'] !== 'com.provar.plugins.forcedotcom.core.ui.UiDoAction') return false; - const locator = getArgUri(call, 'locator')?.toLowerCase() ?? ''; - if (locator.includes('action%3dsave') || locator.includes('action=save')) return true; - const interaction = getArgUri(call, 'interaction')?.toLowerCase() ?? ''; - return /[?&]name=save(&|$)/.test(locator) && interaction.includes('name=action'); + return getLocatorActionToken(call) === 'save'; } /** The direct steps inside a UiWithScreen's `` block (in document order). */ @@ -1053,11 +1078,16 @@ function flagAssertsOnNewScreen(screen: XmlNode, rule: BPRule, out: BPViolation[ } /** - * Heuristic 2 — any UI action step that appears AFTER a Save click inside the SAME - * UiWithScreen substeps block. Save navigates away, so trailing steps are suspect. + * Heuristic 2 — any UI action step that appears AFTER a plain Save click inside the + * SAME create-screen (`action=New`) substeps block. Scoped to `action=New` because + * the rationale is screen teardown: the create form is destroyed on Save and the + * user lands on the saved record's View screen. On `action=Edit`/`View` a post-save + * step on the same record page is a normal, valid pattern and must NOT fire. * Emits one violation per offending trailing step. */ function flagStepsAfterSave(screen: XmlNode, rule: BPRule, out: BPViolation[]): void { + const action = (getScreenAction(getUiWithScreenTargetUri(screen)) ?? '').toLowerCase(); + if (action !== 'new') return; const calls = getScreenSubstepCalls(screen); let sawSave = false; for (const step of calls) { @@ -1070,29 +1100,47 @@ function flagStepsAfterSave(screen: XmlNode, rule: BPRule, out: BPViolation[]): out.push( makeViolation( rule, - `${describeStep(step)} appears after a Save (action=save) inside the same UiWithScreen block. ` + - 'Save navigates away from the screen, so this step runs in stale context. Move it into a new ' + + `${describeStep(step)} appears after a Save (action=save) inside the same action=New screen block. ` + + 'Save tears down the create screen, so this step runs in stale context. Move it into a new ' + 'UiWithScreen targeting action=View (or the appropriate post-save screen).' ) ); } } +/** The testItemId an offending step is associated with, parsed from a violation message (`testItemId=N`). */ +function violationTestItemId(v: BPViolation): string { + return /testItemId=([^\s)]+)/.exec(v.message)?.[1] ?? v.message; +} + /** * UI-SCREEN-CONTEXT-001 — flag UiAssert/UiRead steps left under the `action=New` - * create screen, and any UI action that follows a Save within the same screen - * block. Both heuristics read data already present in the XML (target URIs and - * locator bindings) and emit ONE violation per offending step so that - * (rule_id, test_item_id) de-dups cleanly. + * create screen (heuristic 1), and any UI action that follows a plain Save within + * the same create-screen block (heuristic 2). Both heuristics read data already + * present in the XML (target URIs and locator bindings). + * + * The canonical defect (a UiAssert placed after Save inside the create screen) + * trips BOTH heuristics for the SAME step. Because this local engine sums every + * BPViolation when scoring (it has no test_item_id field to de-dup on, unlike the + * Quality Hub API), we de-dup here and emit AT MOST ONE violation per offending + * testItemId so the step is penalised once (major × weight 5), not twice. */ function validateUiAssertScreenContext(tc: XmlNode, rule: BPRule): BPViolation[] { - const violations: BPViolation[] = []; + const raw: BPViolation[] = []; for (const call of getAllApiCalls(tc)) { if (call['@_apiId'] !== UI_WITH_SCREEN_API_ID) continue; - flagAssertsOnNewScreen(call, rule, violations); - flagStepsAfterSave(call, rule, violations); - } - return violations; + flagAssertsOnNewScreen(call, rule, raw); + flagStepsAfterSave(call, rule, raw); + } + const seen = new Set(); + const deduped: BPViolation[] = []; + for (const v of raw) { + const key = violationTestItemId(v); + if (seen.has(key)) continue; + seen.add(key); + deduped.push(v); + } + return deduped; } // ── NitroX MS variant validators (UI-NITROX-CONNECT-ARGS-001, UI-NITROX-VARIANT-ARG-001) ─── diff --git a/test/unit/mcp/bestPracticesEngine.test.ts b/test/unit/mcp/bestPracticesEngine.test.ts index ffbcb97..d2e12a1 100644 --- a/test/unit/mcp/bestPracticesEngine.test.ts +++ b/test/unit/mcp/bestPracticesEngine.test.ts @@ -440,20 +440,25 @@ describe('runBestPractices', () => { } // ─ POSITIVE: constructed bad fixture (assert under action=New, after Save) fires ─ - it('fires on the constructed bad fixture (UiAssert under action=New, after Save)', async () => { + // The UiAssert (testItemId=14) trips BOTH heuristics; the validator must de-dup + // to exactly ONE violation for that step so the local score matches a single + // major × weight 5 deduction (100 − 5×0.75 = 96.25), not a double penalty. + it('fires exactly once (de-duped) on the constructed bad fixture (UiAssert under action=New, after Save)', async () => { const xml = await readFixture('opportunity-create-verify-bad.testcase'); const result = runBestPractices(xml); const vs = screenViolations(result.violations); - assert.ok(vs.length >= 1, `Expected UI-SCREEN-CONTEXT-001 to fire; got ${vs.length}`); + assert.equal( + vs.length, + 1, + `Expected exactly one (de-duped) violation; got ${JSON.stringify(vs.map((v) => v.message))}` + ); assert.equal(vs[0].severity, 'major'); assert.equal(vs[0].weight, 5); assert.ok( - vs.some((v) => v.message.includes('testItemId=14')), - `Expected a violation naming the offending UiAssert (testItemId=14): ${JSON.stringify( - vs.map((v) => v.message) - )}` + vs[0].message.includes('testItemId=14'), + `Expected the violation to name the offending UiAssert (testItemId=14): ${vs[0].message}` ); - assert.ok(result.quality_score < 100, `Expected quality_score < 100, got ${result.quality_score}`); + assert.equal(result.quality_score, 96.25, `Expected quality_score 96.25, got ${result.quality_score}`); }); // ─ NEGATIVE: good fixture (assert under action=View) does NOT fire ─ @@ -577,6 +582,88 @@ describe('runBestPractices', () => { const vs = screenViolations(runBestPractices(xml).violations); assert.equal(vs.length, 0, `Expected no UI-SCREEN-CONTEXT-001; got: ${JSON.stringify(vs.map((v) => v.message))}`); }); + + // ─ False-positive guard (a): a post-save step/assert under action=Edit must NOT fire ─ + // On an Edit screen the record page is NOT torn down, so asserting / acting after + // Save on the same page is a normal, valid pattern. Heuristic 2 is scoped to New. + it('does not fire for a step or assert that follows Save under an action=Edit screen', () => { + const xml = ` + + + + + + + + + + + + + + + + + + + + + + + + + + + +`; + const vs = screenViolations(runBestPractices(xml).violations); + assert.equal( + vs.length, + 0, + `Expected no UI-SCREEN-CONTEXT-001 under action=Edit; got: ${JSON.stringify(vs.map((v) => v.message))}` + ); + }); + + // ─ False-positive guard (b): a "Save & New" continuation under action=New must NOT fire ─ + // "Save & New" (action=saveAndNew) keeps the user on a fresh create screen, so filling + // the next record after it is legitimate. The exact-token match must exclude saveAndNew. + it('does not fire for a fill that follows a "Save & New" button under action=New (exact-token Save match)', () => { + const xml = ` + + + + + + + + + + + + + + + + + + + + + + + + + + + +`; + const vs = screenViolations(runBestPractices(xml).violations); + assert.equal( + vs.length, + 0, + `Expected no UI-SCREEN-CONTEXT-001 after Save & New; got: ${JSON.stringify(vs.map((v) => v.message))}` + ); + }); }); // ── UI-NITROX-CONNECT-ARGS-001 / UI-NITROX-VARIANT-ARG-001 — NitroX MS variants ──