feat(mcp): implement 7 test-loop improvements from real usage feedback#121
feat(mcp): implement 7 test-loop improvements from real usage feedback#121mrdailey99 merged 2 commits intodevelopfrom
Conversation
Sources: PROVARDX_CLI_FEEDBACK.md from provar-manager-regression session. Source changes: - testCaseValidate.ts: DATA-001 warning for <dataTable> (silently ignored by CLI standalone runner); ASSERT-001 warning for AssertValues with namedValues id="values" format (silently passes as null=null for variable comparisons); improved TC_012/TC_031 suggestion strings naming crypto.randomUUID() and the UUID v4 variant byte rule. - automationTools.ts: filterTestRunOutput() strips com.networknt.schema and logger-lock SEVERE noise before returning testrun response; output_lines_suppressed field added when lines are dropped. - rcaTools.ts: incrementSiblingDirs() detects Results(N) siblings (Provar Increment mode creates siblings, not numeric children); 5 new Salesforce DML error RCA rules: SALESFORCE_VALIDATION, SALESFORCE_PICKLIST, SALESFORCE_REFERENCE, SALESFORCE_ACCESS, SALESFORCE_TRIGGER. Tests (22 new, 626 total): - testCaseValidate.test.ts: DATA-001 (fires/doesn't fire), ASSERT-001 (fires/doesn't fire for correct args/non-AssertValues), TC_012/TC_031 suggestion text. - automationTools.test.ts: filterTestRunOutput pure-function tests (7), testrun output_lines_suppressed handler tests (2). - rcaTools.test.ts: Results(N) sibling detection (2), Salesforce category classification (4). Docs: docs/mcp.md — DATA-001/ASSERT-001 warning rules, Salesforce RCA categories with infrastructure_issues note, output_lines_suppressed field. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Implements several MCP “test loop” improvements driven by real-world Provar CLI usage feedback: adds additional validator warnings/suggestions, reduces noisy CLI output returned from testrun, and improves RCA artifact resolution and classification.
Changes:
- Add new testcase validation warnings (DATA-001, ASSERT-001) and improve UUID v4 suggestion text (TC_012/TC_031).
- Filter Provar testrun stdout to suppress known schema-validator/logger noise and surface
output_lines_suppressed. - Enhance RCA tools: detect Increment-mode
Results(N)sibling dirs and classify common Salesforce DML failures into new RCA categories; update docs and add unit tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/mcp/tools/testCaseValidate.ts | Adds DATA-001 / ASSERT-001 warnings and updates UUID suggestion strings. |
| src/mcp/tools/automationTools.ts | Introduces filterTestRunOutput() and plumbs output_lines_suppressed into testrun responses. |
| src/mcp/tools/rcaTools.ts | Adds Salesforce RCA rules and Increment-mode Results(N) sibling directory resolution. |
| test/unit/mcp/testCaseValidate.test.ts | Adds unit tests covering new warnings and updated suggestion text. |
| test/unit/mcp/automationTools.test.ts | Adds testrun suppression tests and pure-function tests for output filtering. |
| test/unit/mcp/rcaTools.test.ts | Adds tests for Results(N) sibling resolution and Salesforce category classification. |
| docs/mcp.md | Documents new warnings, new RCA categories, and the output_lines_suppressed field/behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/mcp/tools/automationTools.ts
Outdated
| const lines = raw.split('\n'); | ||
| const kept: string[] = []; | ||
| let suppressed = 0; | ||
| let lastKeptWasBlank = false; | ||
|
|
||
| for (const line of lines) { |
There was a problem hiding this comment.
filterTestRunOutput() splits on "\n" only, so Windows CRLF output will leave trailing "\r" on each line. That can prevent NOISE_PATTERNS from matching (and can also break blank-line collapsing / downstream string comparisons). Consider splitting with /\r?\n/ (or normalizing \r\n→\n first) and trimming a trailing \r from each line before pattern checks.
| const lines = raw.split('\n'); | |
| const kept: string[] = []; | |
| let suppressed = 0; | |
| let lastKeptWasBlank = false; | |
| for (const line of lines) { | |
| const lines = raw.split(/\r?\n/); | |
| const kept: string[] = []; | |
| let suppressed = 0; | |
| let lastKeptWasBlank = false; | |
| for (const rawLine of lines) { | |
| const line = rawLine.endsWith('\r') ? rawLine.slice(0, -1) : rawLine; |
There was a problem hiding this comment.
Fixed in b8f3c87 — changed raw.split('\n') to raw.split(/\r?\n/) and added const line = rawLine.endsWith('\r') ? rawLine.slice(0, -1) : rawLine to strip any residual CR before pattern checks. Added a CRLF-specific test (627 passing).
| if (siblings.length > 0) { | ||
| const targetIndex = run_index ?? Math.max(...siblings); | ||
| return { | ||
| results_dir: path.join(path.dirname(resultsBase), `${path.basename(resultsBase)}(${targetIndex})`), | ||
| run_index: targetIndex, |
There was a problem hiding this comment.
resolveResultsLocation() uses run_index directly when selecting a Results(N) sibling. If a caller passes run_index <= 0 (or a negative number), this will construct a path like Results(0)/Results(-1) and report it as the resolved directory. Consider validating run_index is a positive integer at the tool schema level (e.g. z.number().int().positive()) or returning an error when run_index <= 0.
There was a problem hiding this comment.
Fixed in b8f3c87 — both provar.testrun.report.locate and provar.testrun.rca now use z.number().int().positive().optional() for run_index, so the MCP layer rejects 0 and negative values before they reach resolveResultsLocation.
- filterTestRunOutput: split on /\r?\n/ and strip trailing \r so Windows CRLF output does not leave stray \r chars or break noise pattern matching (Copilot suggestion) - rcaTools run_index schema: add .int().positive() validation on both provar.testrun.report.locate and provar.testrun.rca to reject run_index <= 0 before it reaches resolveResultsLocation (Copilot suggestion) - test: add CRLF-safety test for filterTestRunOutput Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sources: PROVARDX_CLI_FEEDBACK.md from provar-manager-regression session.
Source changes:
Tests (22 new, 626 total):
Docs: docs/mcp.md — DATA-001/ASSERT-001 warning rules, Salesforce RCA categories with infrastructure_issues note, output_lines_suppressed field.