From b5ff945bcc7328eabe56d8874deab0b9bef42462 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Thu, 4 Jun 2026 15:03:48 -0500 Subject: [PATCH 1/2] PDX-0: fix(mcp): stop best-practices engine crashing on numeric tag values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: fast-xml-parser parses a numeric element body (e.g. 123456) to a JS number, not a string. Two best-practices validators (detectDuplicatesLiterals and the mustContainArgument value check) read the body as ((node['#text'] ?? '').trim()), which throws "(...).trim is not a function" on a number. The exception propagates out of validateTestCase, so the MCP tool returns VALIDATE_ERROR instead of a result — observed on ~25% of a real 60-file test-case corpus. A safe nodeText() helper that coerces with String() already exists; these two sites simply did not use it. Fix: Route both sites through nodeText() so numeric bodies coerce to string before trim. Add a regression test that a numeric body does not crash runBestPractices. Verified against the corpus: 60/60 files now validate without throwing (previously ~15 crashed). --- src/mcp/tools/bestPracticesEngine.ts | 7 +++++-- test/unit/mcp/bestPracticesEngine.test.ts | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/src/mcp/tools/bestPracticesEngine.ts b/src/mcp/tools/bestPracticesEngine.ts index 8a7c0a6..f8e0ee9 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; diff --git a/test/unit/mcp/bestPracticesEngine.test.ts b/test/unit/mcp/bestPracticesEngine.test.ts index 283a525..7f6340a 100644 --- a/test/unit/mcp/bestPracticesEngine.test.ts +++ b/test/unit/mcp/bestPracticesEngine.test.ts @@ -1043,6 +1043,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"', () => { From f8dad8441a294756e158c71a9daae01039081b47 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Thu, 4 Jun 2026 15:10:38 -0500 Subject: [PATCH 2/2] PDX-0: fix(mcp): scope RENDER-CASE-001 to the six real valueClass values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: validateValueClassCasing (RENDER-CASE-001) only inspects the valueClass attribute, but its valid set carried 14 class="..." tokens (variable, compound, funcCall, value, valueList, and the gt/lt/eq/... operators) plus integer — none of which ever appear as a valueClass, so they were dead entries — while omitting id, a real corpus valueClass (46 uses). A full AllPOCProjects scan confirms exactly six valueClass values exist: string, boolean, decimal, id, date, dateTime. The QH back-end made the identical correction to keep score parity. Fix: Reduce VALUE_CLASS_CASING_VALID to those six (drops the out-of-scope class tokens and integer, adds the missing id) and keep the datetime -> dateTime canonical-casing entry. valueClass="ID"/"Id" now normalizes to id; camelCase dateTime stays canonical. Adds id coverage tests. --- src/mcp/tools/bestPracticesEngine.ts | 31 ++++++++--------------- test/unit/mcp/bestPracticesEngine.test.ts | 22 ++++++++++++++-- 2 files changed, 30 insertions(+), 23 deletions(-) diff --git a/src/mcp/tools/bestPracticesEngine.ts b/src/mcp/tools/bestPracticesEngine.ts index f8e0ee9..43d4eae 100644 --- a/src/mcp/tools/bestPracticesEngine.ts +++ b/src/mcp/tools/bestPracticesEngine.ts @@ -1304,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 7f6340a..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'); }); });