PDX-518: fix(mcp): read UiWithScreen target from uri attribute

src/mcp/tools/bestPracticesEngine.ts

@@ -756,6 +756,15 @@ function validateUniqueResultNames(tc: XmlNode, rule: BPRule): BPViolation | nul
 
 // ── UiWithScreen target URI validator ─────────────────────────────────────────
 
+// Recognised query-param keys on an `sf:ui:target?…` URI. A UiWithScreen SF
+// target is treated as valid when it carries AT LEAST ONE of these keys. The set
+// is corpus-verified against real Provar IDE test cases (CPQ / Billing / sales
+// flows) — a target whose only keys are outside this set (e.g. `?foo=bar`) is
+// flagged as having no recognised SF parameters. PDX-518: extended with the
+// FlexiPage / Visualforce / record-type navigation params the IDE actually emits
+// (`page`, `pageObject`, `flexiPage`, `flexiPath`, `recordType`, `listView`,
+// `quickAction`, `relatedList`, `noOverride`) so activating the attribute reader
+// does not false-positive on legitimate IDE-authored screens.
 const VALID_SF_UI_PARAMS = new Set([
   'object',
   'action',
@@ -766,22 +775,23 @@ const VALID_SF_UI_PARAMS = new Set([
   'lookup',
   'fieldService',
   'tab',
+  // Visualforce-page navigation (e.g. SBQQ__sb, blng__creditInvoice).
+  'page',
+  'pageObject',
+  // Lightning record-page (FlexiPage) targeting.
+  'flexiPage',
+  'flexiPath',
+  // Record-type-scoped navigation.
+  'recordType',
+  // List-view, quick-action and related-list navigation.
+  'listView',
+  'quickAction',
+  'relatedList',
+  // Standard-action override flag carried on `action=New&noOverride=true` forms;
+  // legitimate on its own as a recognised SF nav parameter.
+  'noOverride',
 ]);
 
-/** Read the "target" argument from a UiWithScreen call via the <arguments> wrapper. */
-function getUiWithScreenTarget(call: XmlNode): string | undefined {
-  const argsNode = call['arguments'] as XmlNode | undefined;
-  if (!argsNode || typeof argsNode !== 'object') return undefined;
-  for (const arg of toArr(argsNode['argument'] as XmlNode | XmlNode[])) {
-    if (!arg || typeof arg !== 'object') continue;
-    if (arg['@_id'] !== 'target') continue;
-    const valElem = arg['value'] as XmlNode | undefined;
-    if (!valElem || typeof valElem !== 'object') return undefined;
-    return (valElem['#text'] as string | undefined) ?? undefined;
-  }
-  return undefined;
-}
-
 /** UI-SCREEN-TARGET — validate UiWithScreen target URIs (SF and page object formats). */
 function validateUiWithScreenTarget(tc: XmlNode, rule: BPRule): BPViolation | null {
   const targetApiId = rule.check['apiId'] as string | undefined;
@@ -792,7 +802,7 @@ function validateUiWithScreenTarget(tc: XmlNode, rule: BPRule): BPViolation | nu
     if (!apiId) continue;
     if (targetApiId && !apiId.includes(targetApiId)) continue;
 
-    const target = getUiWithScreenTarget(call);
+    const target = getUiWithScreenTargetUri(call);
     if (!target) continue;
 
     if (target.startsWith('sf:')) {

test/unit/mcp/bestPracticesEngine.test.ts

@@ -206,6 +206,57 @@ describe('runBestPractices', () => {
       );
       assert.ok(uwsViolation, 'Expected uiWithScreenTarget violation for missing pageobjects. prefix');
     });
+
+    // PDX-518: real Provar IDE test cases store the target in the uri= ATTRIBUTE
+    // of <value class="uiTarget" uri="…"/>, not in element #text. These tests
+    // exercise the attribute form so the rule actually fires on IDE-authored XML.
+    function buildUwsAttrXml(uri: string): string {
+      return `<?xml version="1.0" encoding="UTF-8"?>
+<testCase id="tc-uwsattr" guid="${GUID_TC2}" registryId="tc-uwsattr" name="UWS Attr Target Test">
+  <steps>
+    <apiCall guid="${GUID_UWS}" apiId="com.provar.plugins.forcedotcom.core.ui.UiWithScreen" name="With page" testItemId="1">
+      <arguments>
+        <argument id="target">
+          <value class="uiTarget" uri="${uri}"/>
+        </argument>
+      </arguments>
+    </apiCall>
+  </steps>
+</testCase>`;
+    }
+
+    it('passes for a valid SF target in the uri attribute (sf:ui:target?object=Opportunity&action=New)', () => {
+      const result = runBestPractices(
+        buildUwsAttrXml('sf:ui:target?object=Opportunity&amp;action=New&amp;noOverride=true')
+      );
+      const uwsViolation = result.violations.find((v) => v.rule_id.includes('UI-SCREEN-TARGET'));
+      assert.ok(!uwsViolation, `Expected no uiWithScreenTarget violation, got: ${uwsViolation?.message}`);
+    });
+
+    it('passes for a CPQ Visualforce-page SF target (sf:ui:target?page=SBQQ__sb&pageObject=pageobjects.EditQuote)', () => {
+      // PDX-518: real Salesforce CPQ test cases navigate to Visualforce pages via
+      // page=/pageObject= with no object/action key; these must NOT false-fire.
+      const result = runBestPractices(
+        buildUwsAttrXml('sf:ui:target?page=SBQQ__sb&amp;pageObject=pageobjects.EditQuote')
+      );
+      const uwsViolation = result.violations.find((v) => v.rule_id.includes('UI-SCREEN-TARGET'));
+      assert.ok(
+        !uwsViolation,
+        `Expected no uiWithScreenTarget violation for CPQ page target, got: ${uwsViolation?.message}`
+      );
+    });
+
+    it('fires for an SF target in the uri attribute with no recognised SF params', () => {
+      const result = runBestPractices(buildUwsAttrXml('sf:ui:target?foo=bar'));
+      const uwsViolation = result.violations.find((v) => v.rule_id.includes('UI-SCREEN-TARGET'));
+      assert.ok(uwsViolation, 'Expected uiWithScreenTarget violation for SF target with no recognised params');
+    });
+
+    it('fires for a page object target in the uri attribute with bad pageId prefix', () => {
+      const result = runBestPractices(buildUwsAttrXml('ui:pageobject:target?pageId=LoginPage'));
+      const uwsViolation = result.violations.find((v) => v.rule_id.includes('UI-SCREEN-TARGET'));
+      assert.ok(uwsViolation, 'Expected uiWithScreenTarget violation for missing pageobjects. prefix in uri attribute');
+    });
   });
 
   // ── UI-NEST-STRUCT-001 — UI action nesting structure ──