diff --git a/docs/mcp.md b/docs/mcp.md index df88a94..7ae35b4 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 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`). + - **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..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}`; @@ -1091,6 +1087,129 @@ function validateNitroxVariantArgRequired(tc: XmlNode, rule: BPRule): BPViolatio return violations; } +/** + * 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)) { + if (call['@_apiId'] !== targetApiId) continue; + if (callSatisfiesRequiredArg(call, requiredArg, conditionInTitleAllowed)) continue; + offending.push(stepLabel(call)); + } + + if (!offending.length) return null; + 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)`; + // 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 ──────────────────────────────────────────────────── type ValidatorFn = (tc: XmlNode, rule: BPRule) => BPViolation | null; @@ -1107,6 +1226,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..dd8dd6f 100644 --- a/test/unit/mcp/bestPracticesEngine.test.ts +++ b/test/unit/mcp/bestPracticesEngine.test.ts @@ -573,3 +573,163 @@ 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('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 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('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, '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', () => { + 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}`); + }); +});