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"', () => {