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

[mrdailey99]

Summary

• Implements the mustContainArgument check type in the local best-practices engine, unlocking 21 required-argument rules that shipped in provar_best_practices_rules.json but were silently skipped (no registered validator → silent-pass fallback). External/offline MCP users now get these load/exec-blocking checks without the Quality Hub back-end.
• Scope item 1 of PDX-508 only (biggest single-ROI slice). Tiers 2–5, the vendored provar_test_step_schema.json, the Validation Rule Registry doc, and new MCP resources are follow-up slices.

What the validator does

• For each rule, flags every apiCall whose apiId matches check.apiId when the required check.argument is absent or present only as an empty self-closing <argument/> with no <value>.
• Reuses the existing getCallArguments / argumentHasValue helpers so semantics match the NitroX required-argument validators exactly (no drift).
• Exact apiId match plus trailing :variant suffix (NitroX convention); disabled steps skipped; steps nested inside control-flow containers are checked (recursive). One aggregated violation per rule with count = offending step count (scored via the existing log2 formula).

Rules now enforced: control-flow (CONTROL-IF/WHILE/WAITFOR/FOREACH/SWITCH/SLEEP-*), assertions (ASSERT-COMPARISON-001, ASSERT-EXPECTED-001), BDD (BDD-GIVEN/WHEN/THEN-001), Apex/DB (SOQL-QUERY-001, DB-CONNECT-001/002, SQL-QUERY-001/002), web service (REST-CONN-001/002, REST-REQUEST-001).

Jira

https://provartesting.atlassian.net/browse/PDX-508

Test plan

• yarn compile passes
• yarn test:only passes (1356 tests; 7 new reproduce/cleared tests for mustContainArgument)
• node scripts/mcp-smoke.cjs passes (60/60)
• yarn lint passes

Changes

• src/mcp/tools/bestPracticesEngine.ts: add validateMustContainArgument + register it in VALIDATOR_REGISTRY.
• test/unit/mcp/bestPracticesEngine.test.ts: 7 tests — missing arg fires, populated clears, empty self-closing fires, count aggregation, disabled-step skip, non-matching apiId, nested-step recursion.
• docs/mcp.md: document the newly-enforced mustContainArgument rule class under Warning rules.

Notes for reviewers / customer docs

• Behaviour change flagged for public docs: test cases that previously scored clean may now surface these 21 rules. The Provar team maintains docs/provar-mcp-public-docs.md / the University course separately — this changes enforced offline behaviour and may warrant a mirror update.
• Semantics decision to confirm: I treat an empty self-closing <argument/> (present but no <value>) as a violation, matching the sibling NitroX required-arg logic. If the QH back-end uses presence-only semantics for mustContainArgument, flag it and I'll align.
• No new MCP tool/resource and no rule-count references in README, so no smoke TOTAL_EXPECTED or README changes were needed.

[github-actions]

Quality Orchestrator

🟢 LOW · 2 / 100 · All changed files have mapped tests.

---

🧪 Tests to Run · Running 1 of 54 tests

• unit/mcp/bestPracticesEngine.test.ts

▶ Run command

npx vitest run \
  unit/mcp/bestPracticesEngine.test.ts

---

_{⚡ quality-orchestrator  ·  /qo stub <file>  ·  qo analyze-local}

[Copilot]

Pull request overview

Adds local/offline enforcement for mustContainArgument best-practice checks so that required-argument rules in provar_best_practices_rules.json no longer silently pass when Quality Hub is unavailable.

Changes:

• Implemented validateMustContainArgument and registered it in VALIDATOR_REGISTRY so mustContainArgument rules are executed locally.
• Added unit tests covering missing/empty required arguments, aggregation/counting, disabled-step skipping, apiId non-matches, and nested-step recursion.
• Documented the newly enforced mustContainArgument rule class in docs/mcp.md.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/mcp/tools/bestPracticesEngine.ts	Adds and registers the mustContainArgument validator for local best-practice evaluation.
test/unit/mcp/bestPracticesEngine.test.ts	Adds targeted tests to ensure mustContainArgument behavior matches expectations and handles nesting/aggregation.
docs/mcp.md	Documents the offline/local enforcement of required-argument (mustContainArgument) rules and lists the currently enforced rule IDs.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mrdailey99]

Backend parity validated against quality-hub-agents

Validated the empty-argument semantics (and the rest of the check) against the canonical MustContainArgumentValidator in lambda/src/validator/best_practices_engine.py. Found and corrected four divergences so the local/offline result matches the Lambda:

1. Empty argument = missing (the open question) — backend uses present-AND-non-empty semantics. Now mirrored via argumentHasMeaningfulValue: a variable needs a <path> (or text), funcCall and the comparison/logic operator classes (gt/lt/eq/ne/ge/le/and/or/not) count, compound needs <parts>, simple needs text. A bare <value/> or <value class="variable"/> is a violation.
2. Disabled steps — backend does not skip them; removed the local skip (a missing required arg is load-blocking regardless).
3. Legacy condition-in-title — ported the backend exception where If/DoWhile may carry the condition in the step title (If: {expr}) instead of a condition argument.
4. Score parity — backend returns one violation per rule; switched from aggregated count=N to a single violation with no count, so the weighted-deduction score stays in parity with the Lambda. The message still names every offending step.

Also: exact apiId match (matching the backend's @apiId="…"), and the mustContainArgument test suite expanded to 12 cases covering all of the above.

Copilot nit (redundant requiredContainer ternary): addressed in 4c408da — collapsed to verdict.includes('UiWithRow') ? 'UiWithRow' : 'UiWithScreen'. It was pre-existing logic that prettier reformatting pulled into this diff; the fix is behavior-preserving.