From 403ac21e40ce1f033f6396e7b23460776cbc2f27 Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Mon, 8 Jun 2026 09:37:11 -0500 Subject: [PATCH] PDX-512: fix(mcp): correct TryCatchFinally apiId to control.Finally RCA: docs/PROVAR_TEST_STEP_REFERENCE.md, the bestPracticesEngine VALID_API_IDS allow-list and the loop prompts all referenced control.TryCatchFinally, an apiId that does not exist in Provar (the canonical control-flow step is control.Finally), and the schema also carried a doubled list.ListCompareListCompare Fix: normalized every TryCatchFinally reference to control.Finally across the reference doc, allow-list, comment and prompts; corrected the ListCompare schema apiId; added a data-driven test pinning every documented apiId to the validator allow-list; bumped version to 1.6.1 and synced server.json Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/PROVAR_TEST_STEP_REFERENCE.md | 10 ++-- package.json | 2 +- server.json | 4 +- src/mcp/prompts/loopPrompts.ts | 4 +- src/mcp/rules/provar_test_step_schema.json | 2 +- src/mcp/tools/bestPracticesEngine.ts | 3 +- test/unit/mcp/bestPracticesEngine.test.ts | 59 ++++++++++++++++++++++ 7 files changed, 71 insertions(+), 13 deletions(-) diff --git a/docs/PROVAR_TEST_STEP_REFERENCE.md b/docs/PROVAR_TEST_STEP_REFERENCE.md index eabdad1..db26579 100644 --- a/docs/PROVAR_TEST_STEP_REFERENCE.md +++ b/docs/PROVAR_TEST_STEP_REFERENCE.md @@ -49,7 +49,7 @@ | ForEach | `com.provar.plugins.bundled.apis.control.ForEach` | | DoWhile | `com.provar.plugins.bundled.apis.control.DoWhile` | | WaitFor | `com.provar.plugins.bundled.apis.control.WaitFor` | -| TryCatchFinally | `com.provar.plugins.bundled.apis.control.TryCatchFinally` | +| Finally | `com.provar.plugins.bundled.apis.control.Finally` | | Switch | `com.provar.plugins.bundled.apis.Switch` | | Sleep | `com.provar.plugins.bundled.apis.control.Sleep` | | Fail | `com.provar.plugins.bundled.apis.control.Fail` | @@ -1118,13 +1118,13 @@ Iterates over a list variable. `list` must reference an existing variable. `valu ``` -### TryCatchFinally +### Finally Wraps steps with error handling. The `finally` clause always runs, making it the right place for cleanup steps (e.g., delete records created during the test). ```xml - @@ -1616,5 +1616,5 @@ A callable test (used by `CallTest`) declares its interface via ``, `` wrapping `` wrapping `` elements - [ ] **Data types** — booleans are string `"true"`/`"false"` inside `valueClass="boolean"`; numbers use `valueClass="decimal"` -- [ ] **Cleanup** — either `autoCleanup="true"` on the connection, or explicit `ApexDeleteObject` / `TryCatchFinally` finally block +- [ ] **Cleanup** — either `autoCleanup="true"` on the connection, or explicit `ApexDeleteObject` / `Finally` finally block - [ ] **No hallucinated arguments** — see the Common AI Hallucinations table at the top of this doc diff --git a/package.json b/package.json index aa4b5cf..9b976c6 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "@provartesting/provardx-cli", "description": "A plugin for the Salesforce CLI to orchestrate testing activities and report quality metrics to Provar Quality Hub", - "version": "1.6.0", + "version": "1.6.1", "mcpName": "io.github.ProvarTesting/provar", "license": "BSD-3-Clause", "plugins": [ diff --git a/server.json b/server.json index 88e771f..6e47889 100644 --- a/server.json +++ b/server.json @@ -14,12 +14,12 @@ "url": "https://github.com/ProvarTesting/provardx-cli", "source": "github" }, - "version": "1.6.0", + "version": "1.6.1", "packages": [ { "registryType": "npm", "identifier": "@provartesting/provardx-cli", - "version": "1.6.0", + "version": "1.6.1", "transport": { "type": "stdio" }, diff --git a/src/mcp/prompts/loopPrompts.ts b/src/mcp/prompts/loopPrompts.ts index 394b0e5..9f3a9c0 100644 --- a/src/mcp/prompts/loopPrompts.ts +++ b/src/mcp/prompts/loopPrompts.ts @@ -80,7 +80,7 @@ Follow these steps in order: - The first UiWithScreen must use navigate="Always" - UiAssert requires columnAssertions, pageAssertions, resultScope, captureAfter, beforeWait, autoRetry - Use ApexConnect for the connection step (first step in the test) - - Wrap test body in TryCatchFinally if cleanup is needed (ApexDeleteObject in the finally clause) + - Wrap test body in a Finally block (try/catch/finally) if cleanup is needed (ApexDeleteObject in the finally clause) 5. **Write the file** — save the XML to the tests/ directory in the Provar project. ${projectHint(projectPath)} @@ -252,7 +252,7 @@ Follow these steps in order: **Cleanup** - [ ] Is cleanup handled? Either autoCleanup=true on the connection, or ApexDeleteObject steps, or - a TryCatchFinally with cleanup in the finally clause? + a Finally block with cleanup in the finally clause? **Structure** - [ ] Does the first UiWithScreen use navigate="Always" or "IfNecessary"? diff --git a/src/mcp/rules/provar_test_step_schema.json b/src/mcp/rules/provar_test_step_schema.json index fdcc918..27761cb 100644 --- a/src/mcp/rules/provar_test_step_schema.json +++ b/src/mcp/rules/provar_test_step_schema.json @@ -2523,7 +2523,7 @@ }, "ListCompare": { - "apiId": "com.provar.plugins.bundled.apis.list.ListCompareListCompare", + "apiId": "com.provar.plugins.bundled.apis.list.ListCompare", "description": "Compare two lists", "category": "Utility", "required_arguments": [ diff --git a/src/mcp/tools/bestPracticesEngine.ts b/src/mcp/tools/bestPracticesEngine.ts index e4316f5..ffd49c9 100644 --- a/src/mcp/tools/bestPracticesEngine.ts +++ b/src/mcp/tools/bestPracticesEngine.ts @@ -113,7 +113,6 @@ const VALID_API_IDS = new Set([ 'com.provar.plugins.bundled.apis.control.ActualResult', 'com.provar.plugins.bundled.apis.control.DesignStep', 'com.provar.plugins.bundled.apis.control.CallTest', - 'com.provar.plugins.bundled.apis.control.TryCatchFinally', // Bundled — database 'com.provar.plugins.bundled.apis.db.DbConnect', 'com.provar.plugins.bundled.apis.db.DbDelete', @@ -237,7 +236,7 @@ function toArr(v: T | T[] | undefined | null): T[] { /** * Recursively collect every element in the parsed tree. - * Works for flat and deeply nested structures (StepGroup, BDD, TryCatch…). + * Works for flat and deeply nested structures (StepGroup, BDD, Finally…). */ function getAllApiCalls(node: XmlNode): XmlNode[] { const results: XmlNode[] = []; diff --git a/test/unit/mcp/bestPracticesEngine.test.ts b/test/unit/mcp/bestPracticesEngine.test.ts index 350a5fd..3b501fc 100644 --- a/test/unit/mcp/bestPracticesEngine.test.ts +++ b/test/unit/mcp/bestPracticesEngine.test.ts @@ -1627,3 +1627,62 @@ ${stepsXml} }); }); }); + +// ── reference doc ⇄ validator apiId parity (PDX-512) ────────────────────────── +// Regression guard: every api ID published in docs/PROVAR_TEST_STEP_REFERENCE.md +// (served verbatim as an MCP resource) must be recognised by the validator's +// VALID_API_IDS allow-list. The bogus `...control.TryCatchFinally` slipped +// through precisely because the doc and the allow-list agreed on the same wrong +// value, so existing tests stayed green. This ties the two sources together: any +// future drift fails the build instead of shipping a step Provar cannot load. +describe('reference doc ⇄ validator apiId parity (PDX-512)', () => { + function unknownApiIdViolations(apiId: string): BPViolation[] { + const xml = ` + + + + +`; + return runBestPractices(xml).violations.filter((v) => v.rule_id === 'API-UNKNOWN-001'); + } + + it('recognises every apiId documented in PROVAR_TEST_STEP_REFERENCE.md', async () => { + const fs = await import('node:fs'); + const path = await import('node:path'); + const url = await import('node:url'); + const here = path.dirname(url.fileURLToPath(import.meta.url)); + const docPath = path.join(here, '..', '..', '..', 'docs', 'PROVAR_TEST_STEP_REFERENCE.md'); + const doc = fs.readFileSync(docPath, 'utf-8'); + + // Reference-table rows are shaped `| Name | `com.provar...` |`; section-header + // rows have an empty 2nd cell and are skipped. The captured token keeps any + // NitroX `:ms-*` variant suffix so those resolve against the allow-list too. + const documented = [...new Set([...doc.matchAll(/^\|[^|]+\|\s*`(com\.provar[^`]+)`\s*\|/gm)].map((m) => m[1]))]; + + assert.ok( + documented.length >= 50, + `Expected to parse >=50 apiIds from the reference table, got ${documented.length} — table format may have changed` + ); + + const offenders = documented.filter((apiId) => unknownApiIdViolations(apiId).length > 0); + assert.deepEqual( + offenders, + [], + `Documented apiIds missing from VALID_API_IDS (doc/validator drift): ${offenders.join(', ')}` + ); + }); + + it('still flags the bogus control.TryCatchFinally apiId (the bug this guards)', () => { + const vs = unknownApiIdViolations('com.provar.plugins.bundled.apis.control.TryCatchFinally'); + assert.equal(vs.length, 1, 'API-UNKNOWN-001 must flag the non-existent TryCatchFinally apiId'); + assert.ok(vs[0].message.includes('TryCatchFinally'), `Violation should name the offending apiId: ${vs[0].message}`); + }); + + it('accepts the canonical control.Finally apiId', () => { + assert.equal( + unknownApiIdViolations('com.provar.plugins.bundled.apis.control.Finally').length, + 0, + 'control.Finally is the canonical try/catch/finally step and must be recognised' + ); + }); +});