PDX-508: enforce mustContainArgument best-practice rules in the local validator

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 `<clause name="substeps">` 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 `<path>` 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 `<parts>` 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 `<argument/>`, an empty `<value/>`, or a bare `<value class="variable"/>` with no `<path>`). A value counts as present when it has text, a `funcCall`, a comparison/logic operator (`gt`/`lt`/`eq`/…), a `variable` with a `<path>`, or a `compound` with `<parts>`. 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**
 

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 <clauses><clause name="substeps"><steps> 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 `<value class="gt">`). Mirrors the backend operator allow-list.
+ */
+const MEANINGFUL_VALUE_OPERATOR_CLASSES: ReadonlySet<string> = new Set([
+  'gt',
+  'lt',
+  'eq',
+  'ne',
+  'ge',
+  'le',
+  'and',
+  'or',
+  'not',
+]);
+
+/**
+ * True when an `<argument>` carries a *meaningful* value, mirroring the Quality
+ * Hub `MustContainArgumentValidator` content checks exactly. A `variable` value
+ * counts only if it references a `<path>` (or has non-empty text) — a bare
+ * `<value class="variable"/>` is effectively empty; `funcCall` and the comparison
+ * / logic operator classes always count; `compound` counts only if `<parts>` has
+ * children; any other (simple) value counts only if it has non-empty text. An
+ * `<argument>` with no `<value>` child, or an empty `<value/>`, 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<XmlNode | string>)) {
+    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 <value class="variable"/> — 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 `<argument id=…>` for a call, tolerating both the `<arguments>` 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: `'<title|name>' (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 `<argument id="check.argument">`. 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
+ * `<argument/>`/`<value/>` 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<string, ValidatorFn> = {
   detectDuplicatesLiterals: validateDetectDuplicatesLiterals,
   uniqueResultNames: validateUniqueResultNames,
   uiWithScreenTarget: validateUiWithScreenTarget,
+  mustContainArgument: validateMustContainArgument,
   // 'regex' is dispatched separately (needs metadata)
   // 'uiActionNestingStructure' is dispatched separately (emits one violation per offending step)
 };

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 <apiCall> XML.
+  function buildTc(stepsXml: string): string {
+    return `<?xml version="1.0" encoding="UTF-8"?>
+<testCase id="tc-mca" guid="${GUID_MCA_TC}" registryId="tc-mca" name="MCA Test">
+  <steps>
+${stepsXml}
+  </steps>
+</testCase>`;
+  }
+
+  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(`    <apiCall guid="${GUID_MCA_S1}" apiId="${IF_API}" name="If no condition" testItemId="1"/>`);
+    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(`    <apiCall guid="${GUID_MCA_S1}" apiId="${IF_API}" name="If ok" testItemId="1">
+      <arguments>
+        <argument id="condition">
+          <value class="value" valueClass="string">{{count}} == 1</value>
+        </argument>
+      </arguments>
+    </apiCall>`);
+    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(`    <apiCall guid="${GUID_MCA_S1}" apiId="${IF_API}" name="If empty" testItemId="1">
+      <arguments>
+        <argument id="condition"/>
+      </arguments>
+    </apiCall>`);
+    const v = find(runBestPractices(xml).violations, 'CONTROL-IF-001');
+    assert.ok(v, 'Expected CONTROL-IF-001 to fire when condition has no <value>');
+  });
+
+  // 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(`    <apiCall guid="${GUID_MCA_S1}" apiId="${ASSERT_API}" name="Assert A" testItemId="1"/>
+    <apiCall guid="${GUID_MCA_S2}" apiId="${ASSERT_API}" name="Assert B" testItemId="2"/>`);
+    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(`    <apiCall guid="${GUID_MCA_S1}" apiId="${IF_API}" name="If disabled" testItemId="1">
+      <tags>
+        <string>disabled</string>
+      </tags>
+    </apiCall>`);
+    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 <value> element is empty (present-and-non-empty semantics)', () => {
+    const xml = buildTc(`    <apiCall guid="${GUID_MCA_S1}" apiId="${IF_API}" name="If empty value" testItemId="1">
+      <arguments>
+        <argument id="condition"><value class="value" valueClass="string"/></argument>
+      </arguments>
+    </apiCall>`);
+    const v = find(runBestPractices(xml).violations, 'CONTROL-IF-001');
+    assert.ok(v, 'Expected CONTROL-IF-001 to fire for an empty <value/> (mirrors the back-end)');
+  });
+
+  it('passes when the condition is a variable reference with a <path> child', () => {
+    const xml = buildTc(`    <apiCall guid="${GUID_MCA_S1}" apiId="${IF_API}" name="If variable" testItemId="1">
+      <arguments>
+        <argument id="condition"><value class="variable"><path element="isActive"/></value></argument>
+      </arguments>
+    </apiCall>`);
+    const v = find(runBestPractices(xml).violations, 'CONTROL-IF-001');
+    assert.ok(!v, `A variable reference with a <path> is a valid condition, got: ${v?.message}`);
+  });
+
+  it('fires for a bare <value class="variable"/> with no path or text (effectively empty)', () => {
+    const xml = buildTc(`    <apiCall guid="${GUID_MCA_S1}" apiId="${IF_API}" name="If empty variable" testItemId="1">
+      <arguments>
+        <argument id="condition"><value class="variable"/></argument>
+      </arguments>
+    </apiCall>`);
+    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(`    <apiCall guid="${GUID_MCA_S1}" apiId="${IF_API}" name="If gt" testItemId="1">
+      <arguments>
+        <argument id="condition"><value class="gt"><left/><right/></value></argument>
+      </arguments>
+    </apiCall>`);
+    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(
+      `    <apiCall guid="${GUID_MCA_S1}" apiId="${IF_API}" name="Legacy If" title="If: {Count(Rows) > 0}" testItemId="1"/>`
+    );
+    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(
+      `    <apiCall guid="${GUID_MCA_S1}" apiId="com.provar.plugins.bundled.apis.SwitchCase" name="Switch case" testItemId="1"/>`
+    );
+    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(`    <apiCall guid="${GUID_MCA_S1}" apiId="com.provar.plugins.bundled.apis.control.ForEach" name="Loop" testItemId="1">
+      <arguments>
+        <argument id="list"><value class="value" valueClass="string">{{rows}}</value></argument>
+        <argument id="valueName"><value class="value" valueClass="string">row</value></argument>
+      </arguments>
+      <clauses>
+        <clause name="substeps" testItemId="2">
+          <steps>
+            <apiCall guid="${GUID_MCA_S2}" apiId="${IF_API}" name="Nested If" testItemId="3"/>
+          </steps>
+        </clause>
+      </clauses>
+    </apiCall>`);
+    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}`);
+  });
+});