feat(mcp): add provar.testcase.step.edit and provar.testrun.rca mode=failures by mrdailey99 · Pull Request #132 · ProvarTesting/provardx-cli

mrdailey99 · 2026-04-24T20:39:20Z

Summary

C2: New provar.testcase.step.edit tool — atomically adds or removes a single <apiCall> step in a Provar .testcase XML file. Writes a .bak before any mutation and auto-restores if post-edit validation fails.
C3: Adds mode=failures to provar.testrun.rca — returns a lightweight [{ testItemId, title, errorMessage }] array without the full RCA classification/recommendation overhead. Useful for quick triage.
Security hardening: provar.testrun.rca now enforces path policy on explicit results_path input (previously unchecked).

Changes

File	Change
`src/mcp/tools/testCaseStepTools.ts`	New file — `provar.testcase.step.edit` tool
`src/mcp/tools/rcaTools.ts`	Added `mode` param + path policy on `results_path`
`src/mcp/server.ts`	Register new tool + pass `config` to `registerAllRcaTools`
`test/unit/mcp/testCaseStepTools.test.ts`	11 unit tests (new file)
`test/unit/mcp/rcaTools.test.ts`	4 new tests for `mode=failures`
`scripts/mcp-smoke.cjs`	Entry #49 for `provar.testcase.step.edit`; `TOTAL_EXPECTED=49`
`docs/mcp.md`	Full `provar.testcase.step.edit` section; updated `provar.testrun.rca` section

Notes for reviewer

mode=failures outputs testItemId = tc.name (the JUnit XML string name). JUnit XML does not carry the internal Provar numeric testItemId, so both fields are set to the same string. Field naming is intentional for API symmetry — consumers downstream can look up the actual ID if needed.
Wave 3 sets TOTAL_EXPECTED=49; when Wave 1/2 merge, the conflict resolver bumps to 50 as documented in the multi-wave plan.
Smoke test entry Bump tar from 6.1.11 to 6.2.1 in the npm_and_yarn group across 1 directory #49 cannot be exercised in the local run because the server crashes at entry PDX-294: yml for unpublishing specific publishd version over npm #45 (provar.automation.testrun timeout) — pre-existing issue unrelated to this PR.

Test plan

node_modules/.bin/nyc node_modules/.bin/mocha "test/**/*.test.ts" — 745 passing, 1 pending
yarn compile — clean
yarn lint — clean
Commit hook passed (lint + pretty-quick)

🤖 Generated with Claude Code

…a mode=failures Req: Wave 3 inner-loop tools — atomic test-case step editing with backup/restore safety and lightweight failure extraction from JUnit XML results without full RCA overhead. Fix: Agents editing test steps had to replace entire XML files with no rollback; mode=failures returns a plain failure array for quick triage without loading HTML reports. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

This PR extends the Provar MCP server with (1) a new tool for single-step edits to .testcase XML files and (2) a new lightweight output mode for provar.testrun.rca, along with path-policy hardening for explicit results paths.

Changes:

Added provar.testcase.step.edit tool to add/remove a single <apiCall> step with .bak backup + optional post-edit validation.
Added mode=failures to provar.testrun.rca to return a lightweight failures list (and enforced path policy for explicit results_path).
Updated server registration, docs, smoke script, and unit tests for the new behaviors.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`src/mcp/tools/testCaseStepTools.ts`	Implements the new `provar.testcase.step.edit` MCP tool.
`src/mcp/tools/rcaTools.ts`	Adds `mode` support and path-policy enforcement for `results_path`; updates tool registration signature.
`src/mcp/server.ts`	Registers the new tool and passes `config` into RCA tool registration.
`test/unit/mcp/testCaseStepTools.test.ts`	New unit tests covering add/remove, error cases, validation restore, and path policy.
`test/unit/mcp/rcaTools.test.ts`	Adds tests for default mode and `mode=failures`, plus `results_path` path-policy behavior.
`scripts/mcp-smoke.cjs`	Adds a smoke entry for the new tool and updates the expected call count.
`docs/mcp.md`	Documents `provar.testcase.step.edit` and the new `provar.testrun.rca` `mode` parameter/output.

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

Copilot · 2026-04-24T20:43:07Z

+    if (!callEl) return { error: 'step_xml must contain an <apiCall> element' };
+    const step = (Array.isArray(callEl) ? (callEl as ApiCallNode[])[0] : callEl) as ApiCallNode;
+    return { step };


parseNewStep silently accepts multiple <apiCall> elements in step_xml and just takes the first one. Since the tool is documented as adding/removing a single step, this should be rejected (e.g., require exactly one <apiCall> and return INVALID_STEP_XML otherwise) to avoid surprising partial edits.

Suggested change

if (!callEl) return { error: 'step_xml must contain an <apiCall> element' };

const step = (Array.isArray(callEl) ? (callEl as ApiCallNode[])[0] : callEl) as ApiCallNode;

return { step };

if (!callEl) return { error: 'step_xml must contain exactly one <apiCall> element' };

const calls = Array.isArray(callEl) ? (callEl as ApiCallNode[]) : [callEl as ApiCallNode];

if (calls.length !== 1) {

return { error: 'step_xml must contain exactly one <apiCall> element' };

}

return { step: calls[0] };

Copilot · 2026-04-24T20:43:07Z

+    [
+      'Atomically add or remove a single step (apiCall) in a Provar XML test case file.',
+      'Prerequisites: the test case must exist and be valid XML.',
+      'For mode=remove: supply test_item_id of the step to remove.',
+      'For mode=add: supply test_item_id of the anchor step, position (before|after, default after),',
+      'and step_xml (the <apiCall ...>...</apiCall> XML fragment for the new step).',
+      'A backup is written to <test_case_path>.bak before any mutation and restored automatically if',
+      'the post-edit validation fails.',
+      'Returns STEP_NOT_FOUND (with all_test_item_ids list) when the target step is absent.',


The tool description says it edits files “Atomically”, but the implementation writes directly to the target path with writeFileSync. A process crash or disk-full mid-write can leave a partially written .testcase (even though a .bak exists). Either adjust the wording to avoid claiming atomicity, or implement an atomic replace strategy (e.g., write to a temp file in the same dir and rename it over the original after validation).

Copilot · 2026-04-24T20:43:07Z

      try {
+        // ── Path policy on explicit results_path ─────────────────────────────
+        if (input.results_path) {
+          try {
+            assertPathAllowed(input.results_path, config.allowedPaths);
+          } catch (pErr: unknown) {
+            const e = pErr as Error & { code?: string };
+            const err = makeError(e instanceof PathPolicyError ? e.code : 'PATH_NOT_ALLOWED', e.message, requestId);
+            return { isError: true, content: [{ type: 'text' as const, text: JSON.stringify(err) }] };
+          }
+        }
+
        // ── Resolve location ─────────────────────────────────────────────────
        const resolved = resolveResultsLocation(input.project_path, input.results_path, input.run_index);


provar.testrun.rca now enforces the path policy for results_path, but project_path is still used for file reads (properties/build.xml/validation reports) without any assertPathAllowed check. For consistency with other tools that accept project_path (e.g. project.inspect / testplan tools), add assertPathAllowed(input.project_path, config.allowedPaths) early in the handler so the tool can’t read outside --allowed-paths.

Copilot · 2026-04-24T20:43:07Z

+        let validation: ReturnType<typeof validateTestCase> | null | undefined;
+        if (input.validate_after_edit) {
+          try {
+            validation = validateTestCase(mutatedXml, path.basename(resolvedPath, '.testcase'));
+          } catch {
+            // treat thrown validation errors as failures
+            validation = null;
+          }
+
+          if (validation && !validation.is_valid) {
+            // Restore from backup
+            fs.writeFileSync(resolvedPath, original, 'utf-8');
+            fs.unlinkSync(bakPath);
+            const err = makeError(
+              'INVALID_XML_AFTER_EDIT',
+              `Validation failed after ${input.mode}; original file restored from backup`,
+              requestId,
+              false,
+              { validation_issues: validation.issues }
+            );
+            return { isError: true, content: [{ type: 'text' as const, text: JSON.stringify(err) }] };
+          }


The validation failure path doesn’t actually treat a thrown validation error as a failure. catch { validation = null; } is followed by if (validation && !validation.is_valid), so a thrown error will skip restore + error response and proceed as success. Update the logic to treat null (or any exception) as a validation failure and restore from the backup before returning INVALID_XML_AFTER_EDIT (and consider surfacing the exception details in details).

Req: Four Copilot review comments on PR #132 were all valid and needed fixing before merge to prevent subtle bugs and security gaps. Fix: Reject step_xml with ≠1 apiCall; use temp→rename write; enforce path policy on project_path in rca; treat thrown validation errors as failures not success. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…iles RCA: Internal phased-rollout terminology leaked into docs and source comments, confusing external users who encountered these references Fix: Replace Phase 1/2 labels with descriptive terms across 5 files; no behaviour change Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Req: Wave 2 merged to develop adding provar.connection.list; Wave 3 needs to incorporate those changes without losing its own additions. Fix: Keep both imports/registrations in server.ts; renumber step.edit smoke entry to #50; update TOTAL_EXPECTED to 50 and tools comment to 39. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings April 24, 2026 20:39

Copilot started reviewing on behalf of mrdailey99 April 24, 2026 20:39 View session

Copilot AI reviewed Apr 24, 2026

View reviewed changes

mrdailey99 and others added 3 commits April 24, 2026 16:14

mrdailey99 merged commit db495d3 into develop Apr 24, 2026
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(mcp): add provar.testcase.step.edit and provar.testrun.rca mode=failures#132

feat(mcp): add provar.testcase.step.edit and provar.testrun.rca mode=failures#132
mrdailey99 merged 4 commits intodevelopfrom
feature/wave3-editing-tools

mrdailey99 commented Apr 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 24, 2026

Uh oh!

Copilot AI Apr 24, 2026

Uh oh!

Copilot AI Apr 24, 2026

Uh oh!

Copilot AI Apr 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-    if (!callEl) return { error: 'step_xml must contain an <apiCall> element' };
-    const step = (Array.isArray(callEl) ? (callEl as ApiCallNode[])[0] : callEl) as ApiCallNode;
-    return { step };
+    if (!callEl) return { error: 'step_xml must contain exactly one <apiCall> element' };
+    const calls = Array.isArray(callEl) ? (callEl as ApiCallNode[]) : [callEl as ApiCallNode];
+    if (calls.length !== 1) {
+      return { error: 'step_xml must contain exactly one <apiCall> element' };
+    }
+    return { step: calls[0] };

Conversation

mrdailey99 commented Apr 24, 2026

Summary

Changes

Notes for reviewer

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants