PDX-516: feat(mcp): validate UiAssert screen context (action=New vs View)

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.                        |

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 plain Save** inside the same `action=New` `<clause name="substeps">` 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 `<namedValues>` container.
   - **`SETVALUES-NAME-001`** (`namedValueName`, critical) — a `<namedValue>` inside a `SetValues` step is missing its `name` attribute.

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": [

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"
       },

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 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,
+      "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=<Object>&action=View&recordId={<idVar>}, 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, 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 <clause name=\"substeps\"> 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"
+    },
     {
       "id": "VAR-STRING-LITERAL-001",
       "category": "TestCaseDesign",

src/mcp/tools/bestPracticesEngine.ts

@@ -946,6 +946,203 @@ 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 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;
+// Read-only verification steps that should not run on the create (`action=New`) screen.
+const ASSERT_ON_NEW_SUSPECT_APIS = new Set<string>([
+  '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 (`<value class="uiTarget" uri="…"/>`) 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 `<value>` 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;
+}
+
+/**
+ * Decode the `action` token a UiDoAction locator targets, lower-cased, or undefined.
+ *
+ * The locator URI looks like `ui:locator?name=save&binding=<percent-encoded URI>`,
+ * 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;
+  return getLocatorActionToken(call) === 'save';
+}
+
+/** The direct <apiCall> steps inside a UiWithScreen's `<clause name="substeps"><steps>` 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 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) {
+    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 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 (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 raw: BPViolation[] = [];
+  for (const call of getAllApiCalls(tc)) {
+    if (call['@_apiId'] !== UI_WITH_SCREEN_API_ID) continue;
+    flagAssertsOnNewScreen(call, rule, raw);
+    flagStepsAfterSave(call, rule, raw);
+  }
+  const seen = new Set<string>();
+  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) ───
 
 /**
@@ -2200,6 +2397,7 @@ type MultiValidatorFn = (tc: XmlNode, rule: BPRule) => BPViolation[];
 
 const MULTI_VALIDATOR_REGISTRY: Record<string, MultiValidatorFn> = {
   uiActionNestingStructure: validateUiActionNestingStructure,
+  uiAssertScreenContext: validateUiAssertScreenContext,
   nitroxConnectInvalidArgs: validateNitroxConnectInvalidArgs,
   nitroxVariantArgRequired: validateNitroxVariantArgRequired,
   // Tier 4 — emits one violation per offending value (back-end returns a list)