fix(mcp): best-practices engine corpus-driven fixes (numeric crash + valueClass scope)

src/mcp/tools/bestPracticesEngine.ts

@@ -617,7 +617,9 @@ function validateDetectDuplicatesLiterals(tc: XmlNode, rule: BPRule): BPViolatio
       if (!valElem || typeof valElem !== 'object') continue;
       if ((valElem['@_class'] as string | undefined) !== 'value') continue; // skip variables/compounds
 
-      const text = ((valElem['#text'] as string | undefined) ?? '').trim();
+      // Read through nodeText: fast-xml-parser yields a NUMBER for a numeric tag
+      // value (e.g. <value>123</value>), so a bare `.trim()` would throw.
+      const text = nodeText(valElem);
       if (!text || text.length <= 3) continue;
       if (DEFAULT_LITERAL_VALUES.has(text)) continue;
       if (text.toLowerCase() === 'true' || text.toLowerCase() === 'false') continue;
@@ -1125,7 +1127,8 @@ function argumentHasMeaningfulValue(arg: XmlNode): boolean {
     if (typeof value !== 'object') continue;
     const v = value;
     const vClass = (v['@_class'] as string | undefined) ?? '';
-    const text = ((v['#text'] as string | undefined) ?? '').trim();
+    // nodeText coerces a numeric #text to string first — a bare `.trim()` throws on it.
+    const text = nodeText(v);
 
     if (vClass === 'variable') {
       if (v['path'] != null || text.length > 0) return true;
@@ -1301,36 +1304,25 @@ function directValueText(arg: XmlNode): string {
   return nodeText(v as XmlNode);
 }
 
-// RENDER-CASE-001 — valid valueClass set, EXACTLY as the back-end declares it: the
-// mixed-case `funcCall`/`valueList` members never match a lowercased input, so
-// casing on those two is deliberately never flagged (parity-preserving quirk).
+// RENDER-CASE-001 — the valueClass values that actually exist. This validator only
+// inspects the `valueClass` attribute, and a full-corpus scan (AllPOCProjects) shows
+// exactly SIX distinct valueClass values: string, boolean, decimal, id, date, dateTime
+// — matching the back-end's VALID_VALUE_CLASSES. (The earlier list also carried
+// `class="..."` tokens — variable/compound/funcCall/value/valueList/operators — and
+// `integer`; none of those ever appear as a valueClass, so they were dead entries, and
+// `id` — a real corpus valueClass — was missing. Coordinated with the QH back-end.)
 const VALUE_CLASS_CASING_VALID: ReadonlySet<string> = new Set([
   'string',
   'boolean',
   'decimal',
-  'integer',
+  'id',
   'date',
   'datetime',
-  'variable',
-  'compound',
-  'funcCall',
-  'value',
-  'valueList',
-  'gt',
-  'lt',
-  'eq',
-  'ne',
-  'ge',
-  'le',
-  'and',
-  'or',
-  'not',
 ]);
 
 // Canonical Provar spelling for valueClasses whose correct form is NOT all-lowercase.
-// The corpus and RENDER-DATE-VALUECLASS-001 / VALUE-CLASS-001 all use camelCase `dateTime`,
-// so the lowercase-enforcement below must expect `dateTime`, not `datetime`. (The QH back-end
-// lowercases all classes — flag a matching correction there; PDX-509.)
+// The corpus uses camelCase `dateTime` exclusively (lowercase `datetime` never appears),
+// so the casing check must expect `dateTime`; every other valueClass is all-lowercase.
 const VALUE_CLASS_CANONICAL_CASE: Record<string, string> = { datetime: 'dateTime' };
 
 /** RENDER-CASE-001 — a known valueClass spelled with wrong case (e.g. `Boolean` → `boolean`). */

test/unit/mcp/bestPracticesEngine.test.ts

@@ -783,12 +783,30 @@ ${stepsXml}
       assert.ok(!v, `Lowercase valueClass should pass, got: ${v?.message}`);
     });
 
-    it('does not fire for funcCall/valueList casing (back-end parity quirk: lowercased form is not in the valid set)', () => {
+    it('does not fire for class-attribute tokens used as a valueClass (e.g. FuncCall) — out of scope', () => {
+      // funcCall/valueList/variable/operators are `class="..."` values, never `valueClass`
+      // values, so they are not in the corpus-confirmed valueClass set and are not normalized.
       const v = find(
         runBestPractices(buildValueStep('<value class="value" valueClass="FuncCall">x</value>')).violations,
         'RENDER-CASE-001'
       );
-      assert.ok(!v, 'valueClass="FuncCall" must NOT fire — mirrors the back-end (funcCall.lower() is not in the set)');
+      assert.ok(!v, 'valueClass="FuncCall" must NOT fire — funcCall is a class value, not a valueClass');
+    });
+
+    it('normalizes valueClass="ID" → "id" (id is a real corpus valueClass)', () => {
+      const v = find(
+        runBestPractices(buildValueStep('<value class="value" valueClass="ID">001x</value>')).violations,
+        'RENDER-CASE-001'
+      );
+      assert.ok(v, 'Expected RENDER-CASE-001 to fire for valueClass="ID"');
+      assert.ok(v?.message.includes("'id'"), `Message should suggest lowercase 'id': ${v?.message}`);
+    });
+
+    it('passes for camelCase dateTime and does not fire for already-correct id', () => {
+      const r = runBestPractices(
+        buildValueStep('<value class="value" valueClass="dateTime">1736899200000</value>')
+      ).violations.find((x) => x.rule_id === 'RENDER-CASE-001');
+      assert.ok(!r, 'valueClass="dateTime" is canonical and must pass');
     });
   });
 
@@ -1043,6 +1061,21 @@ ${stepsXml}
     </apiCall>`);
   }
 
+  // ── numeric tag-value robustness (no .trim crash) ──────────────────────────
+  describe('numeric <value> text does not crash the engine', () => {
+    it('runs cleanly when a value element holds a bare number (parsed as a JS number)', () => {
+      // fast-xml-parser yields a NUMBER for <value>123456</value>; a bare `.trim()`
+      // on that previously threw "(...).trim is not a function" and turned real
+      // validation into a VALIDATE_ERROR. nodeText coerces to string first.
+      const xml = buildArgStep('<value class="value" valueClass="decimal">123456</value>');
+      let result: ReturnType<typeof runBestPractices> | undefined;
+      assert.doesNotThrow(() => {
+        result = runBestPractices(xml);
+      }, 'numeric tag value must not crash runBestPractices');
+      assert.ok(result && typeof result.quality_score === 'number');
+    });
+  });
+
   // ── varStringLiteral (VAR-STRING-LITERAL-001) — multi-violation ─────────────
   describe('varStringLiteral — VAR-STRING-LITERAL-001', () => {
     it('fires for a {Var} stored as class="value" valueClass="string"', () => {