diff --git a/src/mcp/tools/bestPracticesEngine.ts b/src/mcp/tools/bestPracticesEngine.ts index 8a7c0a6..43d4eae 100644 --- a/src/mcp/tools/bestPracticesEngine.ts +++ b/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. 123), 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 = 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 = { datetime: 'dateTime' }; /** RENDER-CASE-001 — a known valueClass spelled with wrong case (e.g. `Boolean` → `boolean`). */ diff --git a/test/unit/mcp/bestPracticesEngine.test.ts b/test/unit/mcp/bestPracticesEngine.test.ts index 283a525..5f5b335 100644 --- a/test/unit/mcp/bestPracticesEngine.test.ts +++ b/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('x')).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('001x')).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('1736899200000') + ).violations.find((x) => x.rule_id === 'RENDER-CASE-001'); + assert.ok(!r, 'valueClass="dateTime" is canonical and must pass'); }); }); @@ -1043,6 +1061,21 @@ ${stepsXml} `); } + // ── numeric tag-value robustness (no .trim crash) ────────────────────────── + describe('numeric 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 123456; 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('123456'); + let result: ReturnType | 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"', () => {