From 3e7a47e1f993ed6484a86e8a71ba2d47196603f8 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 3 Jun 2026 15:06:58 -0500 Subject: [PATCH 1/3] PDX-508: feat(mcp): enforce mustContainArgument best-practice rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: 21 required-argument rules (control-flow conditions, assertion comparison/expected values, BDD descriptions, SOQL/SQL/DB/REST connection+result args) shipped in provar_best_practices_rules.json with check.type "mustContainArgument", but the local engine never registered that check type — every one hit the silent-pass fallback and never fired, so external MCP users got none of these load/exec-blocking checks offline. Fix: implement validateMustContainArgument in bestPracticesEngine.ts and register it; it flags steps whose apiId matches check.apiId when the required check.argument is absent or an empty self-closing , reusing the existing argumentHasValue/getCallArguments helpers for parity with the NitroX required-arg validators. Exact apiId match (plus :variant suffix), disabled steps skipped, nested control-flow steps checked, aggregated one-violation-per-rule with count. Adds 7 reproduce/cleared unit tests and documents the newly-enforced rule class in docs/mcp.md. --- docs/mcp.md | 6 ++ src/mcp/tools/bestPracticesEngine.ts | 46 +++++++++- test/unit/mcp/bestPracticesEngine.test.ts | 105 ++++++++++++++++++++++ 3 files changed, 155 insertions(+), 2 deletions(-) diff --git a/docs/mcp.md b/docs/mcp.md index df88a94..94f27f7 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1068,6 +1068,12 @@ Validates an XML test case for schema correctness (validity score) and best prac `UiWithRow` plays a dual role: it is itself a UI action that must be nested, and a container whose `` satisfies the rule for its descendants. Mirrors Quality Hub's `UiActionNestingStructureValidator`. - **VAR-REF-001** — An argument value looks like a variable reference (`{VarName}` or `{Obj.Field}`) but is stored as `class="value" valueClass="string"`. Provar will treat it as a literal string, not resolve the variable. Replace with `class="variable"` and `` elements. - **VAR-REF-002** — A `{VarName}` token is embedded inside a larger plain string (e.g. `SELECT Id FROM Account WHERE Id = '{AccountId}'`). Provar does not perform `{…}` interpolation in string values at runtime; the braces are emitted literally. Use `class="compound"` with `` children to split the literal text and variable references. In `provar_testcase_generate`, pass the value with `{VarName}` placeholders — the generator emits compound XML automatically. +- **Required-argument rules** (`mustContainArgument`) — A step type that requires a named argument is flagged when that argument is absent or left as an empty self-closing `` with no ``. Each rule pins one `(step apiId, required argument)` pair; disabled steps and non-matching step types are skipped, and steps nested inside control-flow containers are checked too. One aggregated violation per rule, with `count` = number of offending steps. These rules run offline / in `local_fallback` so the missing argument is caught without the Quality Hub back-end. Currently enforced: + - **Control flow** — `CONTROL-IF-001` (`If` → `condition`), `CONTROL-WHILE-001` (`DoWhile` → `condition`), `CONTROL-WAITFOR-001`/`-002` (`WaitFor` → `condition` / `maxIterations`), `CONTROL-FOREACH-001`/`-002` (`ForEach` → `list` / `valueName`), `CONTROL-SWITCH-001` (`Switch` → `value`), `CONTROL-SLEEP-002` (`Sleep` → `sleepSecs`). + - **Assertions** — `ASSERT-COMPARISON-001` (`AssertValues` → `comparisonType`), `ASSERT-EXPECTED-001` (`AssertValues` → `expectedValue`). + - **BDD** — `BDD-GIVEN-001` / `BDD-WHEN-001` / `BDD-THEN-001` (`Given`/`When`/`Then` → `description`). + - **Apex / database** — `SOQL-QUERY-001` (`ApexSoqlQuery` → `soqlQuery`), `DB-CONNECT-001`/`-002` (`DbConnect` → `connectionName` / `resultName`), `SQL-QUERY-001`/`-002` (`SqlQuery` → `query` / `dbConnectionName`). + - **Web service** — `REST-CONN-001`/`-002` (`WebConnect` → `connectionName` / `resultName`), `REST-REQUEST-001` (`RestRequest` → `connectionName`). **Error codes** diff --git a/src/mcp/tools/bestPracticesEngine.ts b/src/mcp/tools/bestPracticesEngine.ts index 184867d..94a15b4 100644 --- a/src/mcp/tools/bestPracticesEngine.ts +++ b/src/mcp/tools/bestPracticesEngine.ts @@ -938,8 +938,8 @@ function validateUiActionNestingStructure(tc: XmlNode, rule: BPRule): BPViolatio const requiredContainer = verdict.includes('UiWithRow') ? 'UiWithRow' : verdict.includes('UiWithScreen') - ? 'UiWithScreen' - : 'UiWithScreen'; + ? 'UiWithScreen' + : 'UiWithScreen'; const message = `${shortApi} '${title}' is ${verdict} - must be nested inside a parent ` + `${requiredContainer}'s block${tidSuffix}`; @@ -1091,6 +1091,47 @@ function validateNitroxVariantArgRequired(tc: XmlNode, rule: BPRule): BPViolatio return violations; } +/** + * mustContainArgument — every apiCall whose apiId matches `check.apiId` must carry + * a populated ``. Mirrors the Quality Hub backend + * `mustContainArgument` check: for the targeted step types a required argument + * that is absent (or an empty self-closing `` with no ``) is a + * load/exec-blocking defect. Uses the same presence test as the NitroX + * required-argument validators (`argumentHasValue`) so semantics never drift. + * + * apiId matching is exact, also accepting a trailing `:variant` suffix (the + * NitroX convention) so a variant apiId is never silently skipped. Disabled + * steps are ignored — they do not execute. One aggregated violation per rule, + * with `count` = number of offending steps (scored via log2 like its siblings). + */ +function validateMustContainArgument(tc: XmlNode, rule: BPRule): BPViolation | null { + const targetApiId = (rule.check['apiId'] as string | undefined) ?? ''; + const requiredArg = (rule.check['argument'] as string | undefined) ?? ''; + if (!targetApiId || !requiredArg) return null; + + const offending: string[] = []; + for (const call of getAllApiCalls(tc)) { + const apiId = call['@_apiId'] as string | undefined; + if (!apiId) continue; + if (apiId !== targetApiId && !apiId.startsWith(`${targetApiId}:`)) continue; + if (isDisabledCall(call)) continue; + + const arg = getCallArguments(call).find((a) => a['@_id'] === requiredArg); + if (arg && argumentHasValue(arg)) continue; + + const label = (call['@_title'] as string | undefined) ?? (call['@_name'] as string | undefined) ?? '(unnamed)'; + const tid = call['@_testItemId'] as string | undefined; + offending.push(`'${label}'${tid ? ` (testItemId=${tid})` : ''}`); + } + + if (!offending.length) return null; + let msg = `${offending.length} step(s) missing required '${requiredArg}' argument: ${offending + .slice(0, 2) + .join(', ')}`; + if (offending.length > 2) msg += ` (and ${offending.length - 2} more)`; + return makeViolation(rule, msg, offending.length); +} + // ── Validator dispatch map ──────────────────────────────────────────────────── type ValidatorFn = (tc: XmlNode, rule: BPRule) => BPViolation | null; @@ -1107,6 +1148,7 @@ const VALIDATOR_REGISTRY: Record = { detectDuplicatesLiterals: validateDetectDuplicatesLiterals, uniqueResultNames: validateUniqueResultNames, uiWithScreenTarget: validateUiWithScreenTarget, + mustContainArgument: validateMustContainArgument, // 'regex' is dispatched separately (needs metadata) // 'uiActionNestingStructure' is dispatched separately (emits one violation per offending step) }; diff --git a/test/unit/mcp/bestPracticesEngine.test.ts b/test/unit/mcp/bestPracticesEngine.test.ts index 4c2b0bb..ca77d90 100644 --- a/test/unit/mcp/bestPracticesEngine.test.ts +++ b/test/unit/mcp/bestPracticesEngine.test.ts @@ -573,3 +573,108 @@ describe('runBestPractices', () => { }); }); }); + +// ── mustContainArgument validator (PDX-508) ────────────────────────────────── + +describe('mustContainArgument validator', () => { + const GUID_MCA_TC = '550e8400-e29b-41d4-a716-4466554409a0'; + const GUID_MCA_S1 = '550e8400-e29b-41d4-a716-4466554409a1'; + const GUID_MCA_S2 = '550e8400-e29b-41d4-a716-4466554409a2'; + + // Build a single-step (or multi-step) test case from raw XML. + function buildTc(stepsXml: string): string { + return ` + + +${stepsXml} + +`; + } + + function find(violations: BPViolation[], ruleId: string): BPViolation | undefined { + return violations.find((v) => v.rule_id === ruleId); + } + + // CONTROL-IF-001 — If steps must have a 'condition' argument (critical / weight 8). + const IF_API = 'com.provar.plugins.bundled.apis.If'; + + it('fires when an If step is missing its required condition argument', () => { + const xml = buildTc(` `); + const v = find(runBestPractices(xml).violations, 'CONTROL-IF-001'); + assert.ok(v, 'Expected CONTROL-IF-001 to fire for an If step with no condition'); + assert.equal(v?.severity, 'critical'); + assert.ok(v?.message.includes('condition'), `Message should name the argument: ${v?.message}`); + assert.ok(v?.message.includes('testItemId=1'), `Message should locate the step: ${v?.message}`); + }); + + it('passes when the If step has a populated condition argument', () => { + const xml = buildTc(` + + + {{count}} == 1 + + + `); + const v = find(runBestPractices(xml).violations, 'CONTROL-IF-001'); + assert.ok(!v, `Expected no CONTROL-IF-001 violation, got: ${v?.message}`); + }); + + it('fires when the required argument is present but an empty self-closing tag', () => { + const xml = buildTc(` + + + + `); + const v = find(runBestPractices(xml).violations, 'CONTROL-IF-001'); + assert.ok(v, 'Expected CONTROL-IF-001 to fire when condition has no '); + }); + + // ASSERT-COMPARISON-001 — AssertValues must have a 'comparisonType' argument. + const ASSERT_API = 'com.provar.plugins.bundled.apis.AssertValues'; + + it('aggregates multiple offending steps into one violation with a count', () => { + const xml = buildTc(` + `); + const v = find(runBestPractices(xml).violations, 'ASSERT-COMPARISON-001'); + assert.ok(v, 'Expected ASSERT-COMPARISON-001 to fire'); + assert.equal(v?.count, 2, 'Expected count to reflect both offending AssertValues steps'); + }); + + it('does not fire for a disabled step missing the argument', () => { + const xml = buildTc(` + + disabled + + `); + const v = find(runBestPractices(xml).violations, 'CONTROL-IF-001'); + assert.ok(!v, `Disabled steps should be skipped, got: ${v?.message}`); + }); + + it('does not fire for a different apiId that happens to lack the argument', () => { + const xml = buildTc( + ` ` + ); + const v = find(runBestPractices(xml).violations, 'CONTROL-IF-001'); + assert.ok(!v, 'CONTROL-IF-001 must only apply to If steps'); + }); + + it('checks steps nested inside control-flow containers (recursive)', () => { + const xml = + buildTc(` + + {{rows}} + row + + + + + + + + + `); + const v = find(runBestPractices(xml).violations, 'CONTROL-IF-001'); + assert.ok(v, 'Expected CONTROL-IF-001 to fire for an If nested inside a ForEach'); + assert.ok(v?.message.includes('testItemId=3'), `Message should locate the nested step: ${v?.message}`); + }); +}); From 72408d8108efad5a6db75b27ef0bfc9b546a93e0 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 3 Jun 2026 15:35:21 -0500 Subject: [PATCH 2/3] PDX-508: fix(mcp): align mustContainArgument with Quality Hub backend RCA: Validated the local validator against quality-hub-agents MustContainArgumentValidator (best_practices_engine.py) and found four divergences: (1) empty or bare was accepted but the backend treats present-but-empty as missing; (2) disabled steps were skipped but the backend checks them (a missing required arg is load-blocking regardless); (3) the legacy If/DoWhile condition-in-title format was not honored, risking false positives; (4) aggregating with count=N diverged the weighted-deduction score from the Lambda, which returns one violation per rule. Fix: add argumentHasMeaningfulValue mirroring the backend content checks (variable needs path/text, funcCall and gt/lt/eq/ne/ge/le/and/or/not operator classes count, compound needs parts, simple needs text); stop skipping disabled steps; honor the condition-in-title legacy exception for If/DoWhile; emit a single violation per rule with no count so the score stays in parity (message still names every offender); use exact apiId match. Extract callSatisfiesRequiredArg/stepLabel helpers to keep complexity under 20. Updates tests (12 cases) and docs/mcp.md to the backend-faithful semantics. --- docs/mcp.md | 2 +- src/mcp/tools/bestPracticesEngine.ts | 134 +++++++++++++++++----- test/unit/mcp/bestPracticesEngine.test.ts | 67 ++++++++++- 3 files changed, 170 insertions(+), 33 deletions(-) diff --git a/docs/mcp.md b/docs/mcp.md index 94f27f7..7ae35b4 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1068,7 +1068,7 @@ Validates an XML test case for schema correctness (validity score) and best prac `UiWithRow` plays a dual role: it is itself a UI action that must be nested, and a container whose `` satisfies the rule for its descendants. Mirrors Quality Hub's `UiActionNestingStructureValidator`. - **VAR-REF-001** — An argument value looks like a variable reference (`{VarName}` or `{Obj.Field}`) but is stored as `class="value" valueClass="string"`. Provar will treat it as a literal string, not resolve the variable. Replace with `class="variable"` and `` elements. - **VAR-REF-002** — A `{VarName}` token is embedded inside a larger plain string (e.g. `SELECT Id FROM Account WHERE Id = '{AccountId}'`). Provar does not perform `{…}` interpolation in string values at runtime; the braces are emitted literally. Use `class="compound"` with `` children to split the literal text and variable references. In `provar_testcase_generate`, pass the value with `{VarName}` placeholders — the generator emits compound XML automatically. -- **Required-argument rules** (`mustContainArgument`) — A step type that requires a named argument is flagged when that argument is absent or left as an empty self-closing `` with no ``. Each rule pins one `(step apiId, required argument)` pair; disabled steps and non-matching step types are skipped, and steps nested inside control-flow containers are checked too. One aggregated violation per rule, with `count` = number of offending steps. These rules run offline / in `local_fallback` so the missing argument is caught without the Quality Hub back-end. Currently enforced: +- **Required-argument rules** (`mustContainArgument`) — A step type that requires a named argument is flagged when that argument is absent **or present but empty** (a self-closing ``, an empty ``, or a bare `` with no ``). A value counts as present when it has text, a `funcCall`, a comparison/logic operator (`gt`/`lt`/`eq`/…), a `variable` with a ``, or a `compound` with ``. Each rule pins one `(step apiId, required argument)` pair; apiId matching is exact, steps nested inside control-flow containers are checked, and disabled steps are **not** exempt (a missing required argument is load/exec-blocking regardless). `If`/`DoWhile` steps that carry the condition in the step `title` (legacy `If: {expr}` format) instead of a `condition` argument are accepted. One violation per rule (the message names every offending step) — matching the Quality Hub back-end, so the weighted-deduction score stays in parity with the Lambda. These rules run offline / in `local_fallback` so the missing argument is caught without the back-end. Currently enforced: - **Control flow** — `CONTROL-IF-001` (`If` → `condition`), `CONTROL-WHILE-001` (`DoWhile` → `condition`), `CONTROL-WAITFOR-001`/`-002` (`WaitFor` → `condition` / `maxIterations`), `CONTROL-FOREACH-001`/`-002` (`ForEach` → `list` / `valueName`), `CONTROL-SWITCH-001` (`Switch` → `value`), `CONTROL-SLEEP-002` (`Sleep` → `sleepSecs`). - **Assertions** — `ASSERT-COMPARISON-001` (`AssertValues` → `comparisonType`), `ASSERT-EXPECTED-001` (`AssertValues` → `expectedValue`). - **BDD** — `BDD-GIVEN-001` / `BDD-WHEN-001` / `BDD-THEN-001` (`Given`/`When`/`Then` → `description`). diff --git a/src/mcp/tools/bestPracticesEngine.ts b/src/mcp/tools/bestPracticesEngine.ts index 94a15b4..f96839b 100644 --- a/src/mcp/tools/bestPracticesEngine.ts +++ b/src/mcp/tools/bestPracticesEngine.ts @@ -1092,44 +1092,126 @@ function validateNitroxVariantArgRequired(tc: XmlNode, rule: BPRule): BPViolatio } /** - * mustContainArgument — every apiCall whose apiId matches `check.apiId` must carry - * a populated ``. Mirrors the Quality Hub backend - * `mustContainArgument` check: for the targeted step types a required argument - * that is absent (or an empty self-closing `` with no ``) is a - * load/exec-blocking defect. Uses the same presence test as the NitroX - * required-argument validators (`argumentHasValue`) so semantics never drift. - * - * apiId matching is exact, also accepting a trailing `:variant` suffix (the - * NitroX convention) so a variant apiId is never silently skipped. Disabled - * steps are ignored — they do not execute. One aggregated violation per rule, - * with `count` = number of offending steps (scored via log2 like its siblings). + * Value `class` attributes that, when present on a condition/expression argument, + * are themselves meaningful content — comparison and boolean-logic operators that + * Provar emits for `If`/`DoWhile`/`WaitFor` conditions (e.g. `{Count(Rows) > 0}` + * is stored as ``). Mirrors the backend operator allow-list. + */ +const MEANINGFUL_VALUE_OPERATOR_CLASSES: ReadonlySet = new Set([ + 'gt', + 'lt', + 'eq', + 'ne', + 'ge', + 'le', + 'and', + 'or', + 'not', +]); + +/** + * True when an `` carries a *meaningful* value, mirroring the Quality + * Hub `MustContainArgumentValidator` content checks exactly. A `variable` value + * counts only if it references a `` (or has non-empty text) — a bare + * `` is effectively empty; `funcCall` and the comparison + * / logic operator classes always count; `compound` counts only if `` has + * children; any other (simple) value counts only if it has non-empty text. An + * `` with no `` child, or an empty ``, is NOT meaningful + * and is treated as a missing required argument. + */ +function argumentHasMeaningfulValue(arg: XmlNode): boolean { + for (const value of toArr(arg['value'] as XmlNode | string | Array)) { + if (value == null) continue; + if (typeof value === 'string') { + if (value.trim().length > 0) return true; + continue; + } + if (typeof value !== 'object') continue; + const v = value; + const vClass = (v['@_class'] as string | undefined) ?? ''; + const text = ((v['#text'] as string | undefined) ?? '').trim(); + + if (vClass === 'variable') { + if (v['path'] != null || text.length > 0) return true; + continue; // bare — not meaningful + } + if (vClass === 'funcCall' || MEANINGFUL_VALUE_OPERATOR_CLASSES.has(vClass)) return true; + if (vClass === 'compound') { + const parts = v['parts']; + if (parts && typeof parts === 'object' && Object.keys(parts).some((k) => !k.startsWith('@_'))) return true; + continue; + } + if (text.length > 0) return true; + } + return false; +} + +/** Find an `` for a call, tolerating both the `` wrapper and direct children. */ +function findArgumentById(call: XmlNode, argId: string): XmlNode | undefined { + return getCallArguments(call).find((a) => a['@_id'] === argId) ?? getArguments(call).find((a) => a['@_id'] === argId); +} + +/** Human-readable step label for a violation message: `'' (testItemId=N)`. */ +function stepLabel(call: XmlNode): string { + const label = (call['@_title'] as string | undefined) ?? (call['@_name'] as string | undefined) ?? '(unnamed)'; + const tid = call['@_testItemId'] as string | undefined; + return `'${label}'${tid ? ` (testItemId=${tid})` : ''}`; +} + +/** + * True when `call` satisfies a `mustContainArgument` requirement — either the + * argument is present with a meaningful value, or (for `If`/`DoWhile` conditions) + * the legacy condition-in-title format is used. + */ +function callSatisfiesRequiredArg(call: XmlNode, requiredArg: string, conditionInTitleAllowed: boolean): boolean { + const arg = findArgumentById(call, requiredArg); + if (arg && argumentHasMeaningfulValue(arg)) return true; + if (!arg && conditionInTitleAllowed) { + const title = (call['@_title'] as string | undefined) ?? ''; + if (title.includes('{') && title.includes('}')) return true; // legacy condition-in-title format + } + return false; +} + +/** + * mustContainArgument — every apiCall whose apiId equals `check.apiId` must carry a + * populated ``. Faithful TypeScript port of the + * Quality Hub `MustContainArgumentValidator`, so the local (offline) result and + * the back-end agree: present-AND-non-empty semantics via + * {@link argumentHasMeaningfulValue} (an absent argument OR an empty + * ``/`` is a violation); exact apiId match (no substring / + * variant widening); the legacy exception where `If`/`DoWhile` may carry the + * condition in the step `title` (`If: {expr}`) instead of a `condition` argument; + * disabled steps are NOT skipped (a missing required argument is load/exec + * blocking regardless of the disabled flag); and one violation per rule (the + * back-end returns the first offender), so the weighted-deduction score stays in + * parity with the Lambda. The message still names every offending step without + * inflating `count`. */ function validateMustContainArgument(tc: XmlNode, rule: BPRule): BPViolation | null { const targetApiId = (rule.check['apiId'] as string | undefined) ?? ''; const requiredArg = (rule.check['argument'] as string | undefined) ?? ''; if (!targetApiId || !requiredArg) return null; + const conditionInTitleAllowed = + requiredArg === 'condition' && + (targetApiId === 'com.provar.plugins.bundled.apis.If' || + targetApiId === 'com.provar.plugins.bundled.apis.control.DoWhile'); + const offending: string[] = []; for (const call of getAllApiCalls(tc)) { - const apiId = call['@_apiId'] as string | undefined; - if (!apiId) continue; - if (apiId !== targetApiId && !apiId.startsWith(`${targetApiId}:`)) continue; - if (isDisabledCall(call)) continue; - - const arg = getCallArguments(call).find((a) => a['@_id'] === requiredArg); - if (arg && argumentHasValue(arg)) continue; - - const label = (call['@_title'] as string | undefined) ?? (call['@_name'] as string | undefined) ?? '(unnamed)'; - const tid = call['@_testItemId'] as string | undefined; - offending.push(`'${label}'${tid ? ` (testItemId=${tid})` : ''}`); + if (call['@_apiId'] !== targetApiId) continue; + if (callSatisfiesRequiredArg(call, requiredArg, conditionInTitleAllowed)) continue; + offending.push(stepLabel(call)); } if (!offending.length) return null; - let msg = `${offending.length} step(s) missing required '${requiredArg}' argument: ${offending - .slice(0, 2) - .join(', ')}`; + const apiName = targetApiId.split('.').pop() ?? targetApiId; + let msg = `${apiName} step missing required '${requiredArg}' argument: ${offending.slice(0, 2).join(', ')}`; if (offending.length > 2) msg += ` (and ${offending.length - 2} more)`; - return makeViolation(rule, msg, offending.length); + // Intentionally no `count`: the back-end reports a single violation per rule, so + // omitting count keeps the weighted-deduction score in parity with the Lambda. + return makeViolation(rule, msg); } // ── Validator dispatch map ──────────────────────────────────────────────────── diff --git a/test/unit/mcp/bestPracticesEngine.test.ts b/test/unit/mcp/bestPracticesEngine.test.ts index ca77d90..dd8dd6f 100644 --- a/test/unit/mcp/bestPracticesEngine.test.ts +++ b/test/unit/mcp/bestPracticesEngine.test.ts @@ -632,22 +632,77 @@ ${stepsXml} // ASSERT-COMPARISON-001 — AssertValues must have a 'comparisonType' argument. const ASSERT_API = 'com.provar.plugins.bundled.apis.AssertValues'; - it('aggregates multiple offending steps into one violation with a count', () => { + it('emits a single violation per rule for multiple offenders and does not inflate count (score parity)', () => { + // The Quality Hub back-end returns one violation per rule; omitting `count` + // keeps the weighted-deduction score in parity with the Lambda. const xml = buildTc(` `); - const v = find(runBestPractices(xml).violations, 'ASSERT-COMPARISON-001'); - assert.ok(v, 'Expected ASSERT-COMPARISON-001 to fire'); - assert.equal(v?.count, 2, 'Expected count to reflect both offending AssertValues steps'); + const matches = runBestPractices(xml).violations.filter((v) => v.rule_id === 'ASSERT-COMPARISON-001'); + assert.equal(matches.length, 1, 'Expected exactly one ASSERT-COMPARISON-001 violation'); + assert.equal(matches[0].count, undefined, 'count must be unset so the score stays in parity with the back-end'); + assert.ok(matches[0].message.includes('and 0 more') === false); + assert.ok( + matches[0].message.includes('testItemId=1') && matches[0].message.includes('testItemId=2'), + `Message should still name both offenders: ${matches[0].message}` + ); }); - it('does not fire for a disabled step missing the argument', () => { + it('fires for a disabled step missing the argument (back-end does not skip disabled steps)', () => { const xml = buildTc(` disabled `); const v = find(runBestPractices(xml).violations, 'CONTROL-IF-001'); - assert.ok(!v, `Disabled steps should be skipped, got: ${v?.message}`); + assert.ok(v, 'A missing required argument is load-blocking even on a disabled step (matches the back-end)'); + }); + + it('fires when the argument is present but its element is empty (present-and-non-empty semantics)', () => { + const xml = buildTc(` + + + + `); + const v = find(runBestPractices(xml).violations, 'CONTROL-IF-001'); + assert.ok(v, 'Expected CONTROL-IF-001 to fire for an empty (mirrors the back-end)'); + }); + + it('passes when the condition is a variable reference with a child', () => { + const xml = buildTc(` + + + + `); + const v = find(runBestPractices(xml).violations, 'CONTROL-IF-001'); + assert.ok(!v, `A variable reference with a is a valid condition, got: ${v?.message}`); + }); + + it('fires for a bare with no path or text (effectively empty)', () => { + const xml = buildTc(` + + + + `); + const v = find(runBestPractices(xml).violations, 'CONTROL-IF-001'); + assert.ok(v, 'A bare variable value with no path/text is treated as missing (mirrors the back-end)'); + }); + + it('passes when the condition is a comparison-operator value (e.g. class="gt")', () => { + const xml = buildTc(` + + + + `); + const v = find(runBestPractices(xml).violations, 'CONTROL-IF-001'); + assert.ok(!v, `A comparison-operator condition is valid, got: ${v?.message}`); + }); + + it('passes an If with no condition argument when the condition is carried in the title (legacy format)', () => { + const xml = buildTc( + ` ` + ); + const v = find(runBestPractices(xml).violations, 'CONTROL-IF-001'); + assert.ok(!v, `Legacy condition-in-title format should pass, got: ${v?.message}`); }); it('does not fire for a different apiId that happens to lack the argument', () => { From 4c408da11bc5387adffdc62e54290cf4b5587c39 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 3 Jun 2026 15:36:51 -0500 Subject: [PATCH 3/3] PDX-508: refactor(mcp): simplify redundant requiredContainer ternary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: Copilot review flagged a nested ternary in validateUiActionNestingStructure where both non-UiWithRow branches return 'UiWithScreen', so the inner verdict.includes('UiWithScreen') test is dead and the expression is harder to read. Prettier reformatting in this branch surfaced the pre-existing line in the diff. Fix: collapse to verdict.includes('UiWithRow') ? 'UiWithRow' : 'UiWithScreen' — behavior-preserving (the eliminated branch returned the same value) and clearer. No test impact; the nesting message tests still pass. --- src/mcp/tools/bestPracticesEngine.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/mcp/tools/bestPracticesEngine.ts b/src/mcp/tools/bestPracticesEngine.ts index f96839b..a05c188 100644 --- a/src/mcp/tools/bestPracticesEngine.ts +++ b/src/mcp/tools/bestPracticesEngine.ts @@ -935,11 +935,7 @@ function validateUiActionNestingStructure(tc: XmlNode, rule: BPRule): BPViolatio const tid = call['@_testItemId'] as string | undefined; const shortApi = apiId.split('.').pop() ?? apiId; const tidSuffix = tid ? ` (testItemId=${tid})` : ''; - const requiredContainer = verdict.includes('UiWithRow') - ? 'UiWithRow' - : verdict.includes('UiWithScreen') - ? 'UiWithScreen' - : 'UiWithScreen'; + const requiredContainer = verdict.includes('UiWithRow') ? 'UiWithRow' : 'UiWithScreen'; const message = `${shortApi} '${title}' is ${verdict} - must be nested inside a parent ` + `${requiredContainer}'s block${tidSuffix}`;