diff --git a/docs/VALIDATION_RULE_REGISTRY.md b/docs/VALIDATION_RULE_REGISTRY.md index 2e32373..2269776 100644 --- a/docs/VALIDATION_RULE_REGISTRY.md +++ b/docs/VALIDATION_RULE_REGISTRY.md @@ -11,7 +11,7 @@ Provar test-case validation runs in two layers. This registry is the single cano **The validity bridge (PDX-509):** a `critical` best-practice violation is surfaced into `issues[]` as an `ERROR` and therefore gates `is_valid` — EXCEPT where a Layer-1 check already owns the concept (then it is suppressed to avoid double-reporting). `major`/`minor`/`info` affect `quality_score` (and the `needs_improvement` status) only. The `status` field is tri-state: `invalid` (a critical) / `needs_improvement` (loads but `quality_score < quality_threshold`) / `valid`. -**Counts:** Layer 1 — 23 rules (18 gating). Layer 2 — 178 rules (critical 64 / major 67 / minor 29 / info 18; 58 bridged to `is_valid`). +**Counts:** Layer 1 — 23 rules (18 gating). Layer 2 — 179 rules (critical 64 / major 68 / minor 29 / info 18; 58 bridged to `is_valid`). ## Layer 1 — Structural validity rules @@ -194,6 +194,7 @@ Provar test-case validation runs in two layers. This registry is the single cano | `UI-LOCATOR-SAVE-001` | TestCaseDesign | major | 5 | No | Save button locator must use correct pattern. | | `UI-LOOKUP-ID-001` | TestCaseDesign | major | 6 | No | UiDoAction lookup fields should use Name values, not IDs. | | `UI-NAVIGATE-PREFER-SCREEN-001` | TestCaseDesign | info | 1 | No | Prefer UiWithScreen over UiNavigate for Salesforce. | +| `UI-SCREEN-CONTEXT-001` | TestCaseDesign | major | 5 | No | UI verification or post-save step left under the wrong screen context. | | `UI-SCREEN-NAV-001` | TestCaseDesign | major | 5 | No | First UiWithScreen must use navigate=Always or IfNeccessary. | | `UI-SCREEN-NAV-002` | TestCaseDesign | minor | 2 | No | First UiWithScreen should prefer navigate=Always over IfNeccessary. | | `UI-SCREEN-OBJID-001` | TestCaseDesign | major | 5 | No | UiWithScreen with navigate=Always for Edit/View must have sfUiTargetObjectId. | diff --git a/docs/mcp.md b/docs/mcp.md index bf7b87a..a3798c4 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1101,6 +1101,7 @@ Validates an XML test case for schema correctness (validity score) and best prac - **`CONN-DB-002`** (`dbConnectResultNameMismatch`) — a DB operation (`SqlQuery`/`DbRead`/`DbInsert`/`DbUpdate`/`DbDelete`) sets a `dbConnectionName` that does not match any `DbConnect` `resultName` in the test, so the open connection can't be found. Defers to `CONN-DB-001` when there is no `DbConnect` at all. - **`UI-LOCATOR-BUTTON-CASING-001`** (`uiLocatorButtonCasing`) — a `UiDoAction` locator uses a wrong-cased standard-button name: `name=Cancel` (use lowercase `name=cancel`) or `name=Continue`/`name=continue` on the record-type screen (use `name=save&path=selectRecordType`). - **`UI-LOCATOR-RECORDTYPE-001`** (`uiLocatorRecordTypeField`) — a `UiDoAction` Record Type picker locator uses `name=recordTypeId`/`name=recordType` instead of `name=RecordType` with `field=RecordTypeId` in the binding. + - **`UI-SCREEN-CONTEXT-001`** (`uiAssertScreenContext`) — a `UiAssert`/`UiRead` left nested under a create screen (`UiWithScreen` whose target URI carries `action=New`), or any UI action placed **after a plain Save** inside the same `action=New` `` block. Save is detected by parsing the `UiDoAction` locator binding's decoded `action` token and matching it **exactly** to `save` (case-insensitive), so continuation buttons like **Save & New** (`action=saveAndNew`) are not treated as a Save. Salesforce tears down the create form on Save and navigates to the saved record's View screen, so a verification still under `action=New` asserts against a page that no longer exists, and steps following the Save in the same create wrapper run in stale context. **Both checks are scoped to `action=New`** — post-save steps/asserts under `action=Edit`/`View` are a valid same-record pattern and do **not** fire. The test still loads and may score 100, so this is a runtime/logic correctness issue. Fix: close the create screen at the Save step and open a new `UiWithScreen` targeting `action=View` (capturing the new record id via `sfUiTargetResultName`) for the verification or follow-up action. A step that trips both checks (assert placed after Save under New) is **de-duped to one violation per offending step** inside the local engine before scoring, so it is penalised once (major × weight 5 → score 96.25), not twice — this matters because the local engine sums every violation and has no `test_item_id` field of its own. **Coverage limit:** this is a structural heuristic — a passing run confirms the create/verify split is structured correctly, not that the assertion logic itself is correct. - **Structural correctness rules** — Checks on element shape / required structure that the Provar IDE depends on to render and run a step. Mostly critical; like the rules above they run offline / in `local_fallback` and keep score parity with the Quality Hub back-end. Currently enforced: - **`SETVALUES-STRUCTURE-001`** (`setValuesStructure`, critical) — a `SetValues` step has no `` container. - **`SETVALUES-NAME-001`** (`namedValueName`, critical) — a `` inside a `SetValues` step is missing its `name` attribute. @@ -2029,13 +2030,13 @@ The constraint is also referenced in the [`provar_testcase_generate`](#provar_te Tools that surface Salesforce org metadata to authoring tools without making a live API call. These read from data that has already been written to disk by the Provar IDE — they do **not** trigger metadata downloads themselves and they do **not** require an authenticated session. -> **Distinct from `.provarCaches`:** the runtime cache used by `provar_automation_testrun` lives at `/.provarCaches/` and is regenerated per run. The cache read by `provar_org_describe` lives in the Provar IDE **workspace** (`/.metadata//`) and is updated when a user opens the project and loads a connection in the IDE. +> **Distinct from `.provarCaches`:** the runtime cache used by `provar_automation_testrun` lives at `/.provarCaches/` and is regenerated per run. The cache read by `provar_org_describe` lives in the Provar IDE **workspace** `.metadata` directory and is updated when a user opens the project and loads a connection in the IDE. ### `provar_org_describe` Read cached Salesforce describe data for one connection from the Provar workspace `.metadata` cache. Returns the object list, required-field schema, and a cache age. Use this before calling `provar_testcase_generate` so the generator can produce steps with correctly-typed field values. -**Prerequisite:** the project must have been opened in Provar IDE at least once with the named connection loaded. If the cache is missing, the tool returns a structured response with `details.suggestion` rather than an error. +**Prerequisite:** the project must have been opened in Provar IDE at least once with the named connection loaded, and (for the chosen test environment) the relevant objects expanded so their metadata is written to disk. If the cache is missing, the tool returns a structured response with `details.suggestion` rather than an error. **Workspace discovery heuristic** — the tool walks candidate directories in this order and uses the first one that exists: @@ -2045,12 +2046,20 @@ Read cached Salesforce describe data for one connection from the Provar workspac `` is the project's basename with whitespace collapsed to single dashes and lowercased: `"My Project"` → `"my-project"`. -| Input | Type | Required | Default | Description | -| ----------------- | ----------------------- | -------- | ------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| `project_path` | string | yes | — | Absolute path to the Provar test project root (the directory containing `.testproject`). Must be within `--allowed-paths`. | -| `connection_name` | string | yes | — | Connection name as defined in `.testproject` (e.g. `"MyOrg"`). Must match the `.metadata` subdirectory exactly. Path separators in this value are rejected (`PATH_TRAVERSAL`). | -| `objects` | string[] | no | all | Filter — only return data for these object API names. When omitted, lists every object cached under the connection directory. | -| `field_filter` | `'required'` \| `'all'` | no | `'required'` | Which fields to return. `'required'` includes only fields with `nillable=false`; `'all'` returns every cached field. | +**Cache layout resolution (within the discovered workspace).** The tool prefers the Provar IDE SfObject cache and falls back to the legacy/native layout: + +1. **Provar IDE SfObject layout (preferred):** `/.metadata/.plugins/com.provar.eclipse.ui///SfObject/.xml` — one XML file per object, written by the IDE when a connection's objects are expanded. `` is the test environment name (e.g. `default`, `UAT`). The tool tries the requested `environment` first; if that environment has no `SfObject` directory, it falls back to any environment that does, preferring `default`. +2. **Legacy / native layout (fallback):** `/.metadata//.{json,xml,object}` — used only when the IDE SfObject layout is absent. + +`cache_age_ms` reflects the `mtime` of whichever cache directory was used (the `SfObject` directory for the IDE layout). + +| Input | Type | Required | Default | Description | +| ----------------- | ----------------------- | -------- | ------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `project_path` | string | yes | — | Absolute path to the Provar test project root (the directory containing `.testproject`). Must be within `--allowed-paths`. | +| `connection_name` | string | yes | — | Connection name as defined in `.testproject` (e.g. `"MyOrg"`). Must match the cache subdirectory exactly. String identifier, **not** a file path — path separators or `..` in this value are rejected (`PATH_TRAVERSAL`). | +| `environment` | string | no | `'default'` | Test environment name whose cached metadata to read in the Provar IDE SfObject layout (e.g. `"default"`, `"UAT"`). If the requested environment has no cached metadata the tool falls back to any environment that does, preferring `default`. Ignored for the legacy layout. String identifier, **not** a file path — path separators or `..` are rejected (`PATH_TRAVERSAL`). | +| `objects` | string[] | no | all | Filter — only return data for these object API names. When omitted, lists every object cached under the resolved cache directory. | +| `field_filter` | `'required'` \| `'all'` | no | `'required'` | Which fields to return. `'required'` includes only fields with `nillable=false`; `'all'` returns every cached field. | | Output field | Description | | ------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | @@ -2133,7 +2142,27 @@ Read cached Salesforce describe data for one connection from the Provar workspac } ``` -**On-disk cache schema (one file per object).** The tool reads `.json` first, then `.xml`, then `.object` as a fallback: +**On-disk cache schema (one file per object).** + +The **preferred** format is the Provar IDE SfObject XML at `/.metadata/.plugins/com.provar.eclipse.ui///SfObject/.xml`. Fields live under ``: + +```xml + + + + + + + + User + + + +``` + +Field mapping: `name` ← `n` (a field with no `n` is skipped); `type` ← `type` (absent → `"unknown"`; Salesforce booleans appear as `_boolean`); `nillable` ← `NOT(required="true")`; `default_value` is always `null` (this format carries no default). The object display name comes from the `sfObject` `t` attribute, else `n`. A **stub** file (`detailsLoaded="false"` with no ``) is reported as `exists: true, field_count: 0` — not an error. + +The **legacy/native** format (read only when the SfObject layout is absent) lives at `/.metadata//.{json,xml,object}`. JSON is tried first, then `.xml` / `.object` (CustomObject / toolingObjectInfo metadata): ```jsonc // /.metadata//Account.json diff --git a/package.json b/package.json index cd13164..96cb040 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.2", + "version": "1.6.3", "mcpName": "io.github.ProvarTesting/provar", "license": "BSD-3-Clause", "plugins": [ diff --git a/server.json b/server.json index 9b57a70..c32307e 100644 --- a/server.json +++ b/server.json @@ -14,12 +14,12 @@ "url": "https://github.com/ProvarTesting/provardx-cli", "source": "github" }, - "version": "1.6.2", + "version": "1.6.3", "packages": [ { "registryType": "npm", "identifier": "@provartesting/provardx-cli", - "version": "1.6.2", + "version": "1.6.3", "transport": { "type": "stdio" }, diff --git a/src/mcp/rules/provar_best_practices_rules.json b/src/mcp/rules/provar_best_practices_rules.json index 8ba7b00..80248df 100644 --- a/src/mcp/rules/provar_best_practices_rules.json +++ b/src/mcp/rules/provar_best_practices_rules.json @@ -2959,6 +2959,22 @@ "notes": "Mirrors QH UiActionNestingStructureValidator (lambda/src/validator/best_practices_engine.py). Emits one violation per offending step so that (rule_id, test_item_id) de-dups against the API. UiWithRow plays a dual role: itself a UI action that must be nested, and a container whose substeps clause satisfies the rule for its descendants.", "source": "Field observation: flat-emitted UI actions (sibling of UiWithScreen) fail to render in Provar IDE despite scoring 100/94 in earlier local validation" }, + { + "id": "UI-SCREEN-CONTEXT-001", + "category": "TestCaseDesign", + "name": "UI verification or post-save step left under the wrong screen context", + "description": "A UiAssert or UiRead nested under a create screen (UiWithScreen target action=New), or any UI action placed after a plain Save (action=save) inside the same action=New substeps block, is almost certainly bound to the wrong screen. Salesforce tears down the create form on Save and navigates to the saved record's View screen, so a verification still nested under action=New asserts against a page that no longer exists, and steps following the Save in the same create wrapper run in stale context. Both checks are scoped to action=New: post-save steps under action=Edit/View are a valid same-record pattern and do not fire, and continuation buttons like Save & New (action=saveAndNew) are not treated as a Save. The test case still loads and may even score 100, so this is a runtime/logic correctness issue (major), not a load failure.", + "appliesTo": ["Step", "TestCase"], + "severity": "major", + "weight": 5, + "recommendation": "Close the create UiWithScreen at the Save step and open a new UiWithScreen targeting the post-save screen for any verification or follow-up action. For Salesforce record creation, assert the saved record under a UiWithScreen with target sf:ui:target?object=&action=View&recordId={}, capturing the created record id via sfUiTargetResultName on the create screen. Do not place UiAssert/UiRead steps under an action=New screen, and do not leave steps after a Save inside the create screen's substeps block.", + "check": { + "type": "uiAssertScreenContext", + "target": "step" + }, + "notes": "Two heuristics over data already in the XML, both scoped to UiWithScreen target action=New: (1) UiAssert/UiRead nested directly under the create screen; (2) any UI action that appears after a plain Save UiDoAction within the same block. Save is detected by parsing the locator binding's decoded action token and comparing it EXACTLY to 'save' (case-insensitive), so saveAndNew / savedSearch / savepoint never match. A step that trips both heuristics (assert placed after Save under New) is de-duped to a single violation per offending testItemId inside the local engine before scoring, so it is penalised once (major x weight 5); this matters because the local score sums every BPViolation and has no test_item_id field of its own. Coverage limit: this is a structural heuristic — a passing run confirms the create/verify split is structured correctly, not that the assertion logic itself is correct.", + "source": "Field observation: AI-generated create-and-verify tests leave the UiAssert under the action=New screen, scoring 100 while asserting against the wrong page at runtime" + }, { "id": "VAR-STRING-LITERAL-001", "category": "TestCaseDesign", diff --git a/src/mcp/tools/bestPracticesEngine.ts b/src/mcp/tools/bestPracticesEngine.ts index ffd49c9..54b15c2 100644 --- a/src/mcp/tools/bestPracticesEngine.ts +++ b/src/mcp/tools/bestPracticesEngine.ts @@ -63,6 +63,13 @@ export interface BPViolation { recommendation: string; applies_to: string[]; count?: number; + /** + * The offending step's testItemId, when a rule attaches its violation to a + * specific step. Used as a structured de-dup key (e.g. UI-SCREEN-CONTEXT-001) + * instead of parsing the id out of the human-readable message. Absent/`'N/A'`/ + * empty means "no stable id" — such violations are never merged together. + */ + test_item_id?: string; } export interface BPEngineResult { @@ -333,7 +340,7 @@ export function calculateBPScore(violations: BPViolation[]): number { // ── Violation factory ───────────────────────────────────────────────────────── -function makeViolation(rule: BPRule, message: string, count?: number): BPViolation { +function makeViolation(rule: BPRule, message: string, count?: number, testItemId?: string): BPViolation { const v: BPViolation = { rule_id: rule.id, name: rule.name, @@ -346,6 +353,7 @@ function makeViolation(rule: BPRule, message: string, count?: number): BPViolati applies_to: rule.appliesTo ?? [], }; if (count !== undefined && count > 1) v.count = count; + if (testItemId !== undefined) v.test_item_id = testItemId; return v; } @@ -756,6 +764,15 @@ function validateUniqueResultNames(tc: XmlNode, rule: BPRule): BPViolation | nul // ── UiWithScreen target URI validator ───────────────────────────────────────── +// Recognised query-param keys on an `sf:ui:target?…` URI. A UiWithScreen SF +// target is treated as valid when it carries AT LEAST ONE of these keys. The set +// is corpus-verified against real Provar IDE test cases (CPQ / Billing / sales +// flows) — a target whose only keys are outside this set (e.g. `?foo=bar`) is +// flagged as having no recognised SF parameters. PDX-518: extended with the +// FlexiPage / Visualforce / record-type navigation params the IDE actually emits +// (`page`, `pageObject`, `flexiPage`, `flexiPath`, `recordType`, `listView`, +// `quickAction`, `relatedList`, `noOverride`) so activating the attribute reader +// does not false-positive on legitimate IDE-authored screens. const VALID_SF_UI_PARAMS = new Set([ 'object', 'action', @@ -766,22 +783,23 @@ const VALID_SF_UI_PARAMS = new Set([ 'lookup', 'fieldService', 'tab', + // Visualforce-page navigation (e.g. SBQQ__sb, blng__creditInvoice). + 'page', + 'pageObject', + // Lightning record-page (FlexiPage) targeting. + 'flexiPage', + 'flexiPath', + // Record-type-scoped navigation. + 'recordType', + // List-view, quick-action and related-list navigation. + 'listView', + 'quickAction', + 'relatedList', + // Standard-action override flag carried on `action=New&noOverride=true` forms; + // legitimate on its own as a recognised SF nav parameter. + 'noOverride', ]); -/** Read the "target" argument from a UiWithScreen call via the wrapper. */ -function getUiWithScreenTarget(call: XmlNode): string | undefined { - const argsNode = call['arguments'] as XmlNode | undefined; - if (!argsNode || typeof argsNode !== 'object') return undefined; - for (const arg of toArr(argsNode['argument'] as XmlNode | XmlNode[])) { - if (!arg || typeof arg !== 'object') continue; - if (arg['@_id'] !== 'target') continue; - const valElem = arg['value'] as XmlNode | undefined; - if (!valElem || typeof valElem !== 'object') return undefined; - return (valElem['#text'] as string | undefined) ?? undefined; - } - return undefined; -} - /** UI-SCREEN-TARGET — validate UiWithScreen target URIs (SF and page object formats). */ function validateUiWithScreenTarget(tc: XmlNode, rule: BPRule): BPViolation | null { const targetApiId = rule.check['apiId'] as string | undefined; @@ -792,7 +810,7 @@ function validateUiWithScreenTarget(tc: XmlNode, rule: BPRule): BPViolation | nu if (!apiId) continue; if (targetApiId && !apiId.includes(targetApiId)) continue; - const target = getUiWithScreenTarget(call); + const target = getUiWithScreenTargetUri(call); if (!target) continue; if (target.startsWith('sf:')) { @@ -946,6 +964,217 @@ function validateUiActionNestingStructure(tc: XmlNode, rule: BPRule): BPViolatio return violations; } +// ── UI-SCREEN-CONTEXT-001 — assert/post-save steps under the wrong screen ───── +// A UiAssert / UiRead that verifies a persisted record while still nested under +// the `action=New` create screen is almost always wrong context: the create +// screen tears down after Save, so the assertion runs against the wrong page. +// Likewise any UI action that follows a plain Save (`action=save`, compared +// exactly so `Save & New` is excluded) inside the SAME `action=New` screen block +// is suspect — the create screen is torn down, so trailing steps belong in a +// fresh UiWithScreen (typically `action=View`). Both heuristics are scoped to +// `action=New`; post-save steps under `action=Edit`/`View` are a valid pattern. + +// UI action apiIds whose presence AFTER a Save (in the same screen block) is suspect. +const POST_SAVE_SUSPECT_APIS = UI_ACTION_API_IDS; +// Read-only verification steps that should not run on the create (`action=New`) screen. +const ASSERT_ON_NEW_SUSPECT_APIS = new Set([ + 'com.provar.plugins.forcedotcom.core.ui.UiAssert', + 'com.provar.plugins.forcedotcom.core.ui.UiRead', +]); +const UI_WITH_SCREEN_API_ID = 'com.provar.plugins.forcedotcom.core.ui.UiWithScreen'; + +/** + * Read the URI string carried by a UiWithScreen `target` argument. The value is + * stored either as the `uri` attribute (``) or, + * for hand-authored XML, as element text. Returns undefined when absent. + */ +function getUiWithScreenTargetUri(call: XmlNode): string | undefined { + for (const arg of getCallArguments(call)) { + if (arg['@_id'] !== 'target') continue; + const valElem = arg['value'] as XmlNode | undefined; + if (!valElem || typeof valElem !== 'object') return undefined; + const uriAttr = valElem['@_uri'] as string | undefined; + if (uriAttr) return uriAttr; + return (valElem['#text'] as string | undefined) ?? undefined; + } + return undefined; +} + +/** Parse the `action` query parameter from a UiWithScreen target URI (e.g. `…?object=Opportunity&action=New`). */ +function getScreenAction(targetUri: string | undefined): string | undefined { + if (!targetUri || !targetUri.includes('?')) return undefined; + const qs = targetUri.slice(targetUri.indexOf('?') + 1); + return new URLSearchParams(qs).get('action') ?? undefined; +} + +/** Read the `uri` attribute of a named argument's `` element (e.g. locator / interaction). */ +function getArgUri(call: XmlNode, argId: string): string | undefined { + for (const arg of getCallArguments(call)) { + if (arg['@_id'] !== argId) continue; + const valElem = arg['value'] as XmlNode | undefined; + if (!valElem || typeof valElem !== 'object') return undefined; + return (valElem['@_uri'] as string | undefined) ?? undefined; + } + return undefined; +} + +/** + * Decode the `action` token a UiDoAction locator targets, lower-cased, or undefined. + * + * The locator URI looks like `ui:locator?name=save&binding=`, + * where the decoded binding is itself a query string (`sf:ui:binding:object?object=Opportunity&action=save`). + * We parse the outer locator for `binding`, decode it, and read its `action` param; + * we also accept the `name` param as a fallback when no binding action is present. + * Both are returned as EXACT tokens (no substring matching) so `saveAndNew`, + * `savedSearch`, `savepoint`, etc. never masquerade as a plain `save`. + */ +function getLocatorActionToken(call: XmlNode): string | undefined { + const locatorUri = getArgUri(call, 'locator'); + if (!locatorUri || !locatorUri.includes('?')) return undefined; + const locatorParams = new URLSearchParams(locatorUri.slice(locatorUri.indexOf('?') + 1)); + const binding = locatorParams.get('binding'); + if (binding && binding.includes('?')) { + // URLSearchParams already percent-decodes the binding value; parse its own query string. + const bindingAction = new URLSearchParams(binding.slice(binding.indexOf('?') + 1)).get('action'); + if (bindingAction) return bindingAction.toLowerCase(); + } + const nameToken = locatorParams.get('name'); + return nameToken ? nameToken.toLowerCase() : undefined; +} + +/** + * True when a UiDoAction is a plain Save click. The action token is compared + * EXACTLY (case-insensitive) against `save`, so continuation buttons such as + * `Save & New` (`action=saveAndNew` / `name=saveAndNew`) and inline-edit + * variants do NOT count — they keep the user on the create screen, so steps + * after them are legitimate. + */ +function isSaveAction(call: XmlNode): boolean { + if (call['@_apiId'] !== 'com.provar.plugins.forcedotcom.core.ui.UiDoAction') return false; + return getLocatorActionToken(call) === 'save'; +} + +/** The direct steps inside a UiWithScreen's `` block (in document order). */ +function getScreenSubstepCalls(screen: XmlNode): XmlNode[] { + const clauses = screen['clauses'] as XmlNode | undefined; + if (!clauses || typeof clauses !== 'object') return []; + for (const clause of toArr(clauses['clause'] as XmlNode | XmlNode[])) { + if (!clause || typeof clause !== 'object') continue; + if (clause['@_name'] !== 'substeps') continue; + const steps = clause['steps'] as XmlNode | undefined; + if (!steps || typeof steps !== 'object') continue; + return toArr(steps['apiCall'] as XmlNode | XmlNode[]).filter((c) => c && typeof c === 'object'); + } + return []; +} + +/** Human-readable label for an offending step (`'Title' (testItemId=N)`). */ +function describeStep(call: XmlNode): string { + const ctx = stepContext(call); + return `${ctx.apiName} '${ctx.title}' (testItemId=${ctx.tid})`; +} + +/** + * Heuristic 1 — a UiAssert/UiRead whose nearest enclosing UiWithScreen targets the + * create screen (`action=New`). Emits one violation per offending verification step. + */ +function flagAssertsOnNewScreen(screen: XmlNode, rule: BPRule, out: BPViolation[]): void { + const action = (getScreenAction(getUiWithScreenTargetUri(screen)) ?? '').toLowerCase(); + if (action !== 'new') return; + for (const step of getScreenSubstepCalls(screen)) { + const apiId = step['@_apiId'] as string | undefined; + if (!apiId || !ASSERT_ON_NEW_SUSPECT_APIS.has(apiId)) continue; + out.push( + makeViolation( + rule, + `${describeStep(step)} verifies a persisted record while nested under the create screen ` + + '(action=New). The create screen is torn down after Save, so the assertion runs against the ' + + 'wrong page context. Move it into a new UiWithScreen targeting action=View (or the appropriate post-save screen).', + undefined, + stepContext(step).tid + ) + ); + } +} + +/** + * Heuristic 2 — any UI action step that appears AFTER a plain Save click inside the + * SAME create-screen (`action=New`) substeps block. Scoped to `action=New` because + * the rationale is screen teardown: the create form is destroyed on Save and the + * user lands on the saved record's View screen. On `action=Edit`/`View` a post-save + * step on the same record page is a normal, valid pattern and must NOT fire. + * Emits one violation per offending trailing step. + */ +function flagStepsAfterSave(screen: XmlNode, rule: BPRule, out: BPViolation[]): void { + const action = (getScreenAction(getUiWithScreenTargetUri(screen)) ?? '').toLowerCase(); + if (action !== 'new') return; + const calls = getScreenSubstepCalls(screen); + let sawSave = false; + for (const step of calls) { + if (!sawSave) { + if (isSaveAction(step)) sawSave = true; + continue; + } + const apiId = step['@_apiId'] as string | undefined; + if (!apiId || !POST_SAVE_SUSPECT_APIS.has(apiId)) continue; + out.push( + makeViolation( + rule, + `${describeStep(step)} appears after a Save (action=save) inside the same action=New screen block. ` + + 'Save tears down the create screen, so this step runs in stale context. Move it into a new ' + + 'UiWithScreen targeting action=View (or the appropriate post-save screen).', + undefined, + stepContext(step).tid + ) + ); + } +} + +/** testItemId values that do not stably identify a step, so must never be merged across violations. */ +const UNSTABLE_TEST_ITEM_IDS = new Set(['', 'N/A']); + +/** + * UI-SCREEN-CONTEXT-001 — flag UiAssert/UiRead steps left under the `action=New` + * create screen (heuristic 1), and any UI action that follows a plain Save within + * the same create-screen block (heuristic 2). Both heuristics read data already + * present in the XML (target URIs and locator bindings). + * + * The canonical defect (a UiAssert placed after Save inside the create screen) + * trips BOTH heuristics for the SAME step. Because this local engine sums every + * BPViolation when scoring (unlike the Quality Hub API, which de-dups by + * test_item_id), we de-dup here on the structured `BPViolation.test_item_id` + * the flag* helpers attach (the offending step's testItemId), emitting AT MOST + * ONE violation per stable testItemId so the step is penalised once (major × + * weight 5), not twice. + * + * Steps with no stable id (`test_item_id` absent / `'N/A'` / empty) are treated + * as unique and NEVER merged: two genuinely-different offending steps that both + * lack a testItemId must each be reported, rather than collapsing onto a shared + * `N/A` key (the old message-parse dedup silently under-counted them). + */ +function validateUiAssertScreenContext(tc: XmlNode, rule: BPRule): BPViolation[] { + const raw: BPViolation[] = []; + for (const call of getAllApiCalls(tc)) { + if (call['@_apiId'] !== UI_WITH_SCREEN_API_ID) continue; + flagAssertsOnNewScreen(call, rule, raw); + flagStepsAfterSave(call, rule, raw); + } + const seen = new Set(); + const deduped: BPViolation[] = []; + for (const v of raw) { + const id = v.test_item_id ?? ''; + if (UNSTABLE_TEST_ITEM_IDS.has(id)) { + // No stable id — never merge; report each occurrence. + deduped.push(v); + continue; + } + if (seen.has(id)) continue; + seen.add(id); + deduped.push(v); + } + return deduped; +} + // ── NitroX MS variant validators (UI-NITROX-CONNECT-ARGS-001, UI-NITROX-VARIANT-ARG-001) ─── /** @@ -2200,6 +2429,7 @@ type MultiValidatorFn = (tc: XmlNode, rule: BPRule) => BPViolation[]; const MULTI_VALIDATOR_REGISTRY: Record = { uiActionNestingStructure: validateUiActionNestingStructure, + uiAssertScreenContext: validateUiAssertScreenContext, nitroxConnectInvalidArgs: validateNitroxConnectInvalidArgs, nitroxVariantArgRequired: validateNitroxVariantArgRequired, // Tier 4 — emits one violation per offending value (back-end returns a list) diff --git a/src/mcp/tools/orgDescribeTools.ts b/src/mcp/tools/orgDescribeTools.ts index 26d5264..1184c87 100644 --- a/src/mcp/tools/orgDescribeTools.ts +++ b/src/mcp/tools/orgDescribeTools.ts @@ -52,12 +52,17 @@ export interface OrgDescribeResult { * On-disk cache schema (one file per object) consumed by this tool. * The cache writer is Provar IDE; this tool is read-only. * - * Layout: /.metadata//.json + * Two layouts are supported, IDE layout first (preferred), then the legacy/native layout: * - * Each JSON file contains: { name: "Account", fields: [ { name, type, defaultValue, nillable }, ... ] } + * 1. Provar IDE SfObject layout (what the IDE actually writes today): + * /.metadata/.plugins/com.provar.eclipse.ui///SfObject/.xml + * One file per object, root element , fields under . + * `` is the test environment name (e.g. "default", "UAT"); defaults to "default". * - * As a fallback, .xml / .object files (CustomObject metadata) are also accepted - * to ease migration from the legacy Provar IDE Eclipse cache layout. + * 2. Legacy / native layout (kept for backward compatibility): + * /.metadata//.{json,xml,object} + * JSON files contain { name, fields: [ { name, type, defaultValue, nillable }, ... ] }; + * .xml / .object files are CustomObject / toolingObjectInfo metadata. */ interface CachedField { name: string; @@ -133,11 +138,17 @@ export function discoverWorkspace(projectPath: string, allowedPaths: string[] = // ── Cache reading ───────────────────────────────────────────────────────────── +// Single parser instance handles BOTH on-disk XML formats: +// - legacy CustomObject / toolingObjectInfo: repeats directly under the root, +// so `fields` must always be coerced to an array. +// - Provar IDE SfObject: a single container holds repeated elements, +// so `sfField` must also be coerced to an array. +// The two element names never collide within one document, so arraying both is safe. const XML_PARSER = new XMLParser({ ignoreAttributes: false, attributeNamePrefix: '@_', parseAttributeValue: false, - isArray: (name): boolean => name === 'fields', + isArray: (name): boolean => name === 'fields' || name === 'sfField', }); /** Parse a JSON cache file into the canonical CachedObject shape. */ @@ -147,15 +158,57 @@ function readJsonCacheFile(filePath: string): CachedObject { } /** - * Parse a legacy .object XML file (CustomObject metadata) into the canonical shape. + * Parse the Provar IDE SfObject XML format into the canonical shape. + * + * Layout: ... + * Attribute mapping (fast-xml-parser prefixes attributes with `@_`): + * name ← `@_n` (required; a field with no name is skipped). + * type ← `@_type` (may be absent → 'unknown'; SF booleans appear as '_boolean'). + * nillable ← NOT(`@_required === 'true'`) (required="true" → nillable=false). + * default_value ← null (this format carries no defaultValue attribute). + * The display name is taken from the sfObject `@_t`, else `@_n`, else the file basename. + * Stub files (detailsLoaded="false", no ) yield an empty field list — exists=true, + * field_count=0 — rather than an error. + */ +function readSfObjectXml(sfObject: Record, fallbackName: string): CachedObject { + // `name` is the object API name (@_n, e.g. "provar__Person__c"), mirroring a Salesforce + // describeSObjectResult and the native-JSON cache path. The IDE's @_t is a human display + // label that embeds the key prefix ("Person (a0D)") and must not be used as the API name. + const objectApiName = + (sfObject['@_n'] as string | undefined) ?? (sfObject['@_t'] as string | undefined) ?? fallbackName; + + // is arrayed by XML_PARSER; the (single) container holds the children. + const fieldsContainers = sfObject['fields']; + if (!Array.isArray(fieldsContainers) || fieldsContainers.length === 0) { + return { name: objectApiName, fields: [] }; + } + const container = fieldsContainers[0] as Record; + const sfFieldsRaw = container['sfField']; + if (!Array.isArray(sfFieldsRaw)) return { name: objectApiName, fields: [] }; + + const fields: CachedField[] = []; + for (const f of sfFieldsRaw as Array>) { + const name = f['@_n'] as string | undefined; + if (!name) continue; // skip nameless container artefacts + // required="true" → nillable=false; absent → nillable=true (optional). + const isRequired = f['@_required'] === 'true' || f['@_required'] === true; + fields.push({ + name, + type: (f['@_type'] as string | undefined) ?? 'unknown', + defaultValue: null, + nillable: !isRequired, + }); + } + return { name: objectApiName, fields }; +} + +/** + * Parse a legacy CustomObject / toolingObjectInfo .xml/.object file into the canonical shape. * Only extracts the bare minimum the tool needs: field name, type, nillable. */ -function readXmlCacheFile(filePath: string): CachedObject { - const raw = fs.readFileSync(filePath, 'utf-8'); - const parsed = XML_PARSER.parse(raw) as Record; - const root = (parsed['CustomObject'] ?? parsed['toolingObjectInfo'] ?? {}) as Record; +function readCustomObjectXml(root: Record, fallbackName: string): CachedObject { const fieldsRaw = root['fields']; - if (!Array.isArray(fieldsRaw)) return { name: path.basename(filePath, path.extname(filePath)), fields: [] }; + if (!Array.isArray(fieldsRaw)) return { name: fallbackName, fields: [] }; const fields: CachedField[] = []; for (const f of fieldsRaw as Array>) { @@ -175,7 +228,26 @@ function readXmlCacheFile(filePath: string): CachedObject { nillable: !isRequired, }); } - return { name: path.basename(filePath, path.extname(filePath)), fields }; + return { name: fallbackName, fields }; +} + +/** + * Parse an XML cache file into the canonical CachedObject shape. Detects the format from + * the root element: (Provar IDE layout) vs / + * (legacy/native layout). + */ +function readXmlCacheFile(filePath: string): CachedObject { + const raw = fs.readFileSync(filePath, 'utf-8'); + const parsed = XML_PARSER.parse(raw) as Record; + const fallbackName = path.basename(filePath, path.extname(filePath)); + + const sfObject = parsed['sfObject']; + if (sfObject && typeof sfObject === 'object') { + return readSfObjectXml(sfObject as Record, fallbackName); + } + + const root = (parsed['CustomObject'] ?? parsed['toolingObjectInfo'] ?? {}) as Record; + return readCustomObjectXml(root, fallbackName); } /** Look up the cache file for one object, trying .json then .xml. */ @@ -275,45 +347,129 @@ function cacheMissSuggestion(connectionName: string): string { interface DescribeArgs { project_path: string; connection_name: string; + environment?: string; objects?: string[]; field_filter?: 'required' | 'all'; } +/** Eclipse plugin id under which the Provar IDE writes its SfObject metadata cache. */ +const ECLIPSE_UI_PLUGIN = 'com.provar.eclipse.ui'; +const DEFAULT_ENVIRONMENT = 'default'; + +/** True when dir exists and is a directory (any error → false). */ +function isExistingDir(dir: string): boolean { + try { + return fs.existsSync(dir) && fs.statSync(dir).isDirectory(); + } catch { + return false; + } +} + /** - * Resolve & policy-check the workspace + connection directory. - * Returns the connection directory if it exists and is allowed, otherwise null. + * Reject path-shaped connection / environment names outright. A real connection or + * environment name is an identifier (e.g. "MyOrg", "UAT"); any separator or traversal + * segment is almost certainly a misuse or injection attempt. + */ +function assertIdentifier(value: string, fieldName: string): void { + const hasSeparator = value.includes('/') || value.includes('\\'); + const hasTraversal = value === '..' || value.split(/[/\\]+/).includes('..'); + if (hasSeparator || hasTraversal) { + throw new PathPolicyError( + 'PATH_TRAVERSAL', + `Invalid ${fieldName} (must not contain path separators or directory-traversal segments ('..')): ${value}` + ); + } +} + +/** + * Resolve the Provar IDE SfObject cache directory for a connection + environment, if present. + * + * Layout: /.metadata/.plugins/com.provar.eclipse.ui///SfObject + * Resolution: try the requested first; if its SfObject dir is missing, scan the + * connection dir for ANY /SfObject that exists (preferring `default`). + * + * Path policy is enforced on the eclipse.ui dir, the connection dir, and every candidate + * SfObject dir before any further filesystem call against it. Returns null when no usable + * SfObject directory exists under policy. + */ +function resolveSfObjectDir( + resolvedWorkspace: string, + connectionName: string, + environment: string, + allowedPaths: string[] +): string | null { + const pluginDir = path.resolve(resolvedWorkspace, '.metadata', '.plugins', ECLIPSE_UI_PLUGIN); + assertPathAllowed(pluginDir, allowedPaths); + + const connectionDir = path.resolve(pluginDir, connectionName); + assertPathAllowed(connectionDir, allowedPaths); + if (!isExistingDir(connectionDir)) return null; + + // Build ordered env candidates: requested env first, then `default`, then any others. + const requested = path.resolve(connectionDir, environment, 'SfObject'); + assertPathAllowed(requested, allowedPaths); + if (isExistingDir(requested)) return requested; + + let envDirs: string[]; + try { + envDirs = fs.readdirSync(connectionDir); + } catch { + return null; + } + // Prefer `default`, then alphabetical for determinism. + const ordered = [...envDirs].sort((a, b) => { + if (a === DEFAULT_ENVIRONMENT) return -1; + if (b === DEFAULT_ENVIRONMENT) return 1; + return a.localeCompare(b); + }); + for (const env of ordered) { + if (env === environment) continue; // already tried + const candidate = path.resolve(connectionDir, env, 'SfObject'); + try { + assertPathAllowed(candidate, allowedPaths); + } catch { + continue; // outside policy — skip silently + } + if (isExistingDir(candidate)) return candidate; + } + return null; +} + +/** + * Resolve & policy-check the workspace + cache directory. + * + * Prefers the Provar IDE SfObject layout + * (`/.metadata/.plugins/com.provar.eclipse.ui///SfObject`), + * falling back to the legacy/native layout (`/.metadata/`). + * Returns the cache directory if one exists and is allowed, otherwise null. */ function resolveConnectionDir( workspacePath: string | null, connectionName: string, + environment: string, allowedPaths: string[] ): { connectionDir: string | null; resolvedWorkspace: string | null } { if (!workspacePath) return { connectionDir: null, resolvedWorkspace: null }; - // Reject path-shaped connection names outright. A real connection name from a - // .testproject is an identifier (e.g. "MyOrg"); any separator or traversal - // segment is almost certainly a misuse or injection attempt. - const hasSeparator = connectionName.includes('/') || connectionName.includes('\\'); - const hasTraversal = connectionName === '..' || connectionName.split(/[/\\]+/).includes('..'); - if (hasSeparator || hasTraversal) { - throw new PathPolicyError( - 'PATH_TRAVERSAL', - `Invalid connection_name (must not contain path separators or directory-traversal segments ('..')): ${connectionName}` - ); - } + assertIdentifier(connectionName, 'connection_name'); + assertIdentifier(environment, 'environment'); // Path policy: workspace MUST be inside allowed paths before any fs call against it. const resolvedWorkspace = path.resolve(workspacePath); assertPathAllowed(resolvedWorkspace, allowedPaths); - const connectionDir = path.resolve(resolvedWorkspace, '.metadata', connectionName); - // Belt-and-braces check after composition. - assertPathAllowed(connectionDir, allowedPaths); + // Preferred: Provar IDE SfObject layout. + const sfObjectDir = resolveSfObjectDir(resolvedWorkspace, connectionName, environment, allowedPaths); + if (sfObjectDir) return { connectionDir: sfObjectDir, resolvedWorkspace }; - if (!fs.existsSync(connectionDir) || !fs.statSync(connectionDir).isDirectory()) { + // Fallback: legacy/native /.metadata/. + const legacyDir = path.resolve(resolvedWorkspace, '.metadata', connectionName); + // Belt-and-braces check after composition. + assertPathAllowed(legacyDir, allowedPaths); + if (!isExistingDir(legacyDir)) { return { connectionDir: null, resolvedWorkspace }; } - return { connectionDir, resolvedWorkspace }; + return { connectionDir: legacyDir, resolvedWorkspace }; } function buildCacheMissResponse( @@ -364,9 +520,13 @@ export function registerOrgDescribe(server: McpServer, config: ServerConfig): vo [ 'Read cached Salesforce describe data for one connection from the Provar workspace .metadata cache.', 'Prerequisite: the project must have been opened in Provar IDE at least once with the named connection loaded', - '— this tool is read-only and does NOT trigger a metadata download.', + '(and, for the named test environment, the relevant objects expanded) — this tool is read-only and does NOT trigger a metadata download.', 'Workspace discovery tries in order: /workspace-, ', '/Provar_Workspaces/workspace-, then ~/Provar/workspace-.', + 'Within the workspace it prefers the Provar IDE SfObject cache at', + '.metadata/.plugins/com.provar.eclipse.ui///SfObject/.xml', + '(environment defaults to "default"; if the requested environment is absent it falls back to any cached environment),', + 'and falls back to the legacy .metadata//.{json,xml,object} layout.', 'Returns an empty result with details.suggestion when the cache is missing.', 'Distinct from the runtime .provarCaches cache used by test execution.', ].join(' '), @@ -385,10 +545,20 @@ export function registerOrgDescribe(server: McpServer, config: ServerConfig): vo .string() .describe( desc( - 'Connection name as defined in the .testproject file (e.g. "MyOrg"). The .metadata cache subdirectory must match this exactly.', + 'Connection name as defined in the .testproject file (e.g. "MyOrg"). The cache subdirectory must match this exactly. String identifier, NOT a file path — path separators or ".." are rejected (PATH_TRAVERSAL).', 'string, connection name as defined in .testproject' ) ), + environment: z + .string() + .optional() + .default('default') + .describe( + desc( + 'Test environment name whose cached metadata to read in the Provar IDE SfObject layout (e.g. "default", "UAT"). Defaults to "default". If the requested environment has no cached metadata, the tool falls back to any environment that does (preferring "default"). Ignored for the legacy .metadata/ layout. String identifier, NOT a file path — path separators or ".." are rejected (PATH_TRAVERSAL).', + "string, test environment name; default 'default'" + ) + ), objects: z .array(z.string()) .optional() @@ -424,6 +594,7 @@ export function registerOrgDescribe(server: McpServer, config: ServerConfig): vo const { connectionDir, resolvedWorkspace } = resolveConnectionDir( workspacePath, args.connection_name, + args.environment ?? DEFAULT_ENVIRONMENT, config.allowedPaths ); diff --git a/test/fixtures/opportunity-create-verify-bad.testcase b/test/fixtures/opportunity-create-verify-bad.testcase new file mode 100644 index 0000000..55e9ba2 --- /dev/null +++ b/test/fixtures/opportunity-create-verify-bad.testcase @@ -0,0 +1,271 @@ + + + Log into Salesforce as a Sales Representative, create a new Opportunity populating all mandatory fields (Opportunity Name, Close Date, Stage), save it, and verify the Opportunity is created successfully. Precondition: SalesRepConnection is configured. Outcome: a new Opportunity named 'Provar Test Opportunity' exists and its Name is asserted on the saved record. NOTE: this fixture deliberately leaves the UiAssert nested under the action=New create screen, after the Save, to exercise UI-SCREEN-CONTEXT-001. + + + + + SalesRepConnection + + + SalesRepConnection + + + Test + + + LightningSales + + + true + + + true + + + default + + + Fail + + + true + + + true + + + 0339c9e0-a309-44ed-ba42-6525553f9001 + + + + + + + + + + + + + + + + SalesRepConnection + + + + + + Always + + + On SF Opportunity Home screen + + + Default + + + Default + + + false + + + false + + + + + + + + + + + + + + + Click the New button + + + Default + + + false + + + false + + + + + + + + + + + SalesRepConnection + + + + + + Dont + + + On SF Opportunity New screen + + + opportunityId + + + Test + + + Default + + + Default + + + false + + + false + + + + + + + + + + + + + + + Provar Test Opportunity + + + Set the Opportunity Name field to Provar Test Opportunity + + + false + + + false + + + + + + + + + + + + + 12/31/2026 + + + Set the Close Date field to 12/31/2026 + + + false + + + false + + + + + + + + + + + + + Prospecting + + + Set the Stage field to Prospecting + + + false + + + false + + + + + + + + + + + + + Click the Save button + + + full + + + Default + + + + + + + + + + OpportunityNameResult + + + Test + + + + + + + + Provar Test Opportunity + + + + + + + + + + + + + false + + + + + + + + + + + + + diff --git a/test/fixtures/opportunity-create-verify-good.testcase b/test/fixtures/opportunity-create-verify-good.testcase new file mode 100644 index 0000000..44fb8ae --- /dev/null +++ b/test/fixtures/opportunity-create-verify-good.testcase @@ -0,0 +1,324 @@ + + + Log into Salesforce as a Sales Representative, create a new Opportunity populating all mandatory fields (Opportunity Name, Close Date, Stage), save it, and verify the Opportunity is created successfully. Precondition: SalesRepConnection is configured. Outcome: a new Opportunity named 'Provar Test Opportunity' exists and its Name is asserted on the saved record. + + + + + SalesRepConnection + + + SalesRepConnection + + + Test + + + LightningSales + + + true + + + true + + + default + + + Fail + + + true + + + true + + + 0339c9e0-a309-44ed-ba42-6525553f9001 + + + + + + + + + + + + + + + + SalesRepConnection + + + + + + Always + + + On SF Opportunity Home screen + + + Default + + + Default + + + false + + + false + + + + + + + + + + + + + + + Click the New button + + + Default + + + false + + + false + + + + + Whether the mouse should hover over the field before it is clicked. + + + + + + How the click should be executed for Internet Explorer. + + + + com.provar.core.model.base.java.EnumChoiceListValuesSource + com.provar.core.model.ui.api.ClickMethod + + + + + + + + + + + + + + SalesRepConnection + + + + + + Dont + + + On SF Opportunity New screen + + + opportunityId + + + Test + + + Default + + + Default + + + false + + + false + + + + + + + + + + + + + + + Provar Test Opportunity + + + Set the Opportunity Name field to Provar Test Opportunity + + + false + + + false + + + + + + + + + + + + + 12/31/2026 + + + Set the Close Date field to 12/31/2026 + + + false + + + false + + + + + + + + + + + + + Prospecting + + + Set the Stage field to Prospecting + + + false + + + false + + + + + + + + + + + + + Click the Save button + + + full + + + Default + + + + + + + + + + + SalesRepConnection + + + + + + Dont + + + On SF Opportunity View screen + + + Default + + + Default + + + false + + + false + + + + + + + + + + + + OpportunityNameResult + + + Test + + + + + + + + Provar Test Opportunity + + + + + + + + + + + + + false + + + + + + + + + + + + + diff --git a/test/unit/mcp/bestPracticesEngine.test.ts b/test/unit/mcp/bestPracticesEngine.test.ts index 3b501fc..67df39a 100644 --- a/test/unit/mcp/bestPracticesEngine.test.ts +++ b/test/unit/mcp/bestPracticesEngine.test.ts @@ -206,6 +206,57 @@ describe('runBestPractices', () => { ); assert.ok(uwsViolation, 'Expected uiWithScreenTarget violation for missing pageobjects. prefix'); }); + + // PDX-518: real Provar IDE test cases store the target in the uri= ATTRIBUTE + // of , not in element #text. These tests + // exercise the attribute form so the rule actually fires on IDE-authored XML. + function buildUwsAttrXml(uri: string): string { + return ` + + + + + + + + + + +`; + } + + it('passes for a valid SF target in the uri attribute (sf:ui:target?object=Opportunity&action=New)', () => { + const result = runBestPractices( + buildUwsAttrXml('sf:ui:target?object=Opportunity&action=New&noOverride=true') + ); + const uwsViolation = result.violations.find((v) => v.rule_id.includes('UI-SCREEN-TARGET')); + assert.ok(!uwsViolation, `Expected no uiWithScreenTarget violation, got: ${uwsViolation?.message}`); + }); + + it('passes for a CPQ Visualforce-page SF target (sf:ui:target?page=SBQQ__sb&pageObject=pageobjects.EditQuote)', () => { + // PDX-518: real Salesforce CPQ test cases navigate to Visualforce pages via + // page=/pageObject= with no object/action key; these must NOT false-fire. + const result = runBestPractices( + buildUwsAttrXml('sf:ui:target?page=SBQQ__sb&pageObject=pageobjects.EditQuote') + ); + const uwsViolation = result.violations.find((v) => v.rule_id.includes('UI-SCREEN-TARGET')); + assert.ok( + !uwsViolation, + `Expected no uiWithScreenTarget violation for CPQ page target, got: ${uwsViolation?.message}` + ); + }); + + it('fires for an SF target in the uri attribute with no recognised SF params', () => { + const result = runBestPractices(buildUwsAttrXml('sf:ui:target?foo=bar')); + const uwsViolation = result.violations.find((v) => v.rule_id.includes('UI-SCREEN-TARGET')); + assert.ok(uwsViolation, 'Expected uiWithScreenTarget violation for SF target with no recognised params'); + }); + + it('fires for a page object target in the uri attribute with bad pageId prefix', () => { + const result = runBestPractices(buildUwsAttrXml('ui:pageobject:target?pageId=LoginPage')); + const uwsViolation = result.violations.find((v) => v.rule_id.includes('UI-SCREEN-TARGET')); + assert.ok(uwsViolation, 'Expected uiWithScreenTarget violation for missing pageobjects. prefix in uri attribute'); + }); }); // ── UI-NEST-STRUCT-001 — UI action nesting structure ── @@ -420,6 +471,296 @@ describe('runBestPractices', () => { } }); + // ── UI-SCREEN-CONTEXT-001 — assert / post-save under the wrong screen ── + describe('UI-SCREEN-CONTEXT-001 — uiAssertScreenContext validator', () => { + const GUID_TCSC = '550e8400-e29b-41d4-a716-446655440050'; + const GUID_UWSSC = '550e8400-e29b-41d4-a716-446655440051'; + const GUID_DOSC = '550e8400-e29b-41d4-a716-446655440052'; + const GUID_XSC = '550e8400-e29b-41d4-a716-446655440053'; + + function screenViolations(violations: BPViolation[]): BPViolation[] { + return violations.filter((v) => v.rule_id === 'UI-SCREEN-CONTEXT-001'); + } + + async function readFixture(name: string): Promise { + 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)); + return fs.readFileSync(path.join(here, '..', '..', 'fixtures', name), 'utf-8'); + } + + // ─ POSITIVE: constructed bad fixture (assert under action=New, after Save) fires ─ + // The UiAssert (testItemId=14) trips BOTH heuristics; the validator must de-dup + // to exactly ONE violation for that step so the local score matches a single + // major × weight 5 deduction (100 − 5×0.75 = 96.25), not a double penalty. + it('fires exactly once (de-duped) on the constructed bad fixture (UiAssert under action=New, after Save)', async () => { + const xml = await readFixture('opportunity-create-verify-bad.testcase'); + const result = runBestPractices(xml); + const vs = screenViolations(result.violations); + assert.equal( + vs.length, + 1, + `Expected exactly one (de-duped) violation; got ${JSON.stringify(vs.map((v) => v.message))}` + ); + assert.equal(vs[0].severity, 'major'); + assert.equal(vs[0].weight, 5); + assert.ok( + vs[0].message.includes('testItemId=14'), + `Expected the violation to name the offending UiAssert (testItemId=14): ${vs[0].message}` + ); + assert.equal(result.quality_score, 96.25, `Expected quality_score 96.25, got ${result.quality_score}`); + }); + + // ─ NEGATIVE: good fixture (assert under action=View) does NOT fire ─ + it('does not fire on the good fixture (UiAssert under action=View)', async () => { + const xml = await readFixture('opportunity-create-verify-good.testcase'); + const vs = screenViolations(runBestPractices(xml).violations); + assert.equal(vs.length, 0, `Expected no UI-SCREEN-CONTEXT-001; got: ${JSON.stringify(vs.map((v) => v.message))}`); + }); + + // ─ Heuristic 1: UiAssert directly under an action=New screen fires ─ + it('fires for a UiAssert nested under an action=New screen', () => { + const xml = ` + + + + + + + + + + + + + + + + + +`; + const vs = screenViolations(runBestPractices(xml).violations); + assert.equal(vs.length, 1, `Expected one violation; got ${JSON.stringify(vs.map((v) => v.message))}`); + assert.ok(vs[0].message.includes('action=New'), `Message should mention action=New: ${vs[0].message}`); + assert.ok(vs[0].message.includes('testItemId=3'), `Message should name the step: ${vs[0].message}`); + }); + + // ─ Heuristic 2: any UI action after a Save in the same screen block fires ─ + it('fires for a step that follows a Save within the same UiWithScreen block', () => { + const xml = ` + + + + + + + + + + + + + + + + + + + + + + + + + + + +`; + const vs = screenViolations(runBestPractices(xml).violations); + assert.ok( + vs.some((v) => v.message.includes('testItemId=4') && v.message.includes('after a Save')), + `Expected a post-save violation for the trailing step (testItemId=4): ${JSON.stringify( + vs.map((v) => v.message) + )}` + ); + }); + + // ─ Clean minimal: assert under action=View, nothing after Save → no violation ─ + it('does not fire when the assert is under an action=View screen and nothing follows Save', () => { + const xml = ` + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +`; + const vs = screenViolations(runBestPractices(xml).violations); + assert.equal(vs.length, 0, `Expected no UI-SCREEN-CONTEXT-001; got: ${JSON.stringify(vs.map((v) => v.message))}`); + }); + + // ─ False-positive guard (a): a post-save step/assert under action=Edit must NOT fire ─ + // On an Edit screen the record page is NOT torn down, so asserting / acting after + // Save on the same page is a normal, valid pattern. Heuristic 2 is scoped to New. + it('does not fire for a step or assert that follows Save under an action=Edit screen', () => { + const xml = ` + + + + + + + + + + + + + + + + + + + + + + + + + + + +`; + const vs = screenViolations(runBestPractices(xml).violations); + assert.equal( + vs.length, + 0, + `Expected no UI-SCREEN-CONTEXT-001 under action=Edit; got: ${JSON.stringify(vs.map((v) => v.message))}` + ); + }); + + // ─ False-positive guard (b): a "Save & New" continuation under action=New must NOT fire ─ + // "Save & New" (action=saveAndNew) keeps the user on a fresh create screen, so filling + // the next record after it is legitimate. The exact-token match must exclude saveAndNew. + it('does not fire for a fill that follows a "Save & New" button under action=New (exact-token Save match)', () => { + const xml = ` + + + + + + + + + + + + + + + + + + + + + + + + + + + +`; + const vs = screenViolations(runBestPractices(xml).violations); + assert.equal( + vs.length, + 0, + `Expected no UI-SCREEN-CONTEXT-001 after Save & New; got: ${JSON.stringify(vs.map((v) => v.message))}` + ); + }); + + // ─ Regression: two DISTINCT offending steps that both lack a testItemId must NOT merge ─ + // Both UiAsserts render `testItemId=N/A`, so the old message-parse dedup collapsed them + // onto the shared `N/A` key (silent under-count). With structured de-dup on + // BPViolation.test_item_id, an absent/'N/A' id is treated as unique → BOTH are reported. + it('reports BOTH distinct offending steps when neither has a testItemId (no N/A collision)', () => { + const GUID_DOSC2 = '550e8400-e29b-41d4-a716-446655440054'; + const xml = ` + + + + + + + + + + + + + + + + + + +`; + const vs = screenViolations(runBestPractices(xml).violations); + assert.equal( + vs.length, + 2, + `Expected BOTH testItemId-less steps to be reported (no N/A merge); got ${JSON.stringify( + vs.map((v) => v.message) + )}` + ); + assert.ok( + vs.every((v) => v.test_item_id === 'N/A'), + `Both violations should carry the structured N/A test_item_id: ${JSON.stringify(vs.map((v) => v.test_item_id))}` + ); + assert.ok( + vs.some((v) => v.message.includes('Verify Name')) && vs.some((v) => v.message.includes('Verify Amount')), + `Each distinct step should be named: ${JSON.stringify(vs.map((v) => v.message))}` + ); + }); + }); + // ── UI-NITROX-CONNECT-ARGS-001 / UI-NITROX-VARIANT-ARG-001 — NitroX MS variants ── describe('NitroX MS variants (Dynamics 365 + Power Platform)', () => { diff --git a/test/unit/mcp/orgDescribeTools.test.ts b/test/unit/mcp/orgDescribeTools.test.ts index e0b851d..2ac588f 100644 --- a/test/unit/mcp/orgDescribeTools.test.ts +++ b/test/unit/mcp/orgDescribeTools.test.ts @@ -90,6 +90,68 @@ function writeXmlCache( fs.writeFileSync(path.join(connectionDir, `${objectName}${ext}`), xml, 'utf-8'); } +/** + * Resolve the Provar IDE SfObject cache directory for a connection + environment: + * /.metadata/.plugins/com.provar.eclipse.ui///SfObject + */ +function sfObjectDir(workspace: string, connection: string, env: string): string { + return path.join(workspace, '.metadata', '.plugins', 'com.provar.eclipse.ui', connection, env, 'SfObject'); +} + +/** A single spec for the IDE SfObject fixture format. */ +interface SfFieldSpec { + name: string; + type?: string; // omit → no `type` attribute (reader should default to 'unknown') + required?: boolean; // required="true" → nillable=false + /** Emit a child so the field is a container — children must NOT be counted. */ + referenceTo?: string; +} + +/** + * Write a Provar IDE SfObject XML file (sanitized shape based on the real IDE output) into + * /.metadata/.plugins/com.provar.eclipse.ui///SfObject/.xml. + * When `detailsLoaded` is false the file is a self-closing stub with no element. + */ +function writeSfObjectCache( + workspace: string, + connection: string, + env: string, + objectName: string, + displayName: string, + fields: SfFieldSpec[], + detailsLoaded = true +): void { + const dir = sfObjectDir(workspace, connection, env); + fs.mkdirSync(dir, { recursive: true }); + + let xml: string; + if (!detailsLoaded) { + xml = + '\n' + + `\n`; + } else { + const sfFieldsXml = fields + .map((f) => { + const attrs = [`n="${f.name}"`]; + if (f.type !== undefined) attrs.push(`type="${f.type}"`); + if (f.required) attrs.push('required="true"'); + if (f.referenceTo) attrs.push('relationshipName="Owner"'); + const open = ` \n \n ${f.referenceTo}\n \n `; + } + return `${open}/>`; + }) + .join('\n'); + xml = + '\n' + + `\n` + + ` \n${sfFieldsXml}\n \n` + + '\n'; + } + fs.writeFileSync(path.join(dir, `${objectName}.xml`), xml, 'utf-8'); +} + // ── Test setup ───────────────────────────────────────────────────────────────── let tmpRoot: string; @@ -592,3 +654,228 @@ describe('provar_org_describe — connection_name validation', () => { ); }); }); + +// ── (k) Provar IDE SfObject layout ──────────────────────────────────────────── + +describe('provar_org_describe — Provar IDE SfObject layout', () => { + it('(k.1) parses // and reports field_count + required_fields', () => { + // Fixture modelled on the real provar__Person__c.xml: a mix of typed fields, a + // required field, a type-less field, and a container field with . + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + writeSfObjectCache(siblingWorkspace, 'Admin', 'UAT', 'provar__Person__c', 'Person', [ + { name: 'Id', type: 'id' }, + { name: 'OwnerId', type: 'reference', referenceTo: 'User' }, // container: child must not count + { name: 'Name' }, // no type → 'unknown' + { name: 'provar__Email__c', type: 'email', required: true }, // the only required field + { name: 'provar__Start_Date__c', type: 'date' }, + ]); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'Admin', + environment: 'UAT', + objects: ['provar__Person__c'], + field_filter: 'all', + }); + + assert.equal(isError(result), false); + const body = parseText(result); + const objects = body['objects'] as Array<{ + name: string; + exists: boolean | null; + required_fields: Array<{ name: string; type: string; nillable: boolean; default_value: string | null }>; + field_count: number; + }>; + assert.equal(objects.length, 1); + assert.equal(objects[0].exists, true); + // 5 sfFields; the children of must NOT be counted. + assert.equal(objects[0].field_count, 5, 'container children must not be double-counted'); + assert.equal( + objects[0].name, + 'provar__Person__c', + 'object name is the API name from sfObject @n, not the @t label' + ); + + const byName = new Map(objects[0].required_fields.map((f) => [f.name, f])); + assert.equal(byName.get('Name')?.type, 'unknown', 'field with no type attribute → "unknown"'); + assert.equal(byName.get('provar__Email__c')?.nillable, false, 'required="true" → nillable=false'); + assert.equal(byName.get('Id')?.nillable, true, 'absent required → nillable=true'); + assert.equal(byName.get('provar__Email__c')?.default_value, null, 'SfObject format has no defaultValue'); + }); + + it('(k.2) field_filter=required returns only the required field', () => { + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + writeSfObjectCache(siblingWorkspace, 'Admin', 'UAT', 'provar__Person__c', 'Person', [ + { name: 'Id', type: 'id' }, + { name: 'provar__Email__c', type: 'email', required: true }, + { name: 'Name' }, + ]); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'Admin', + environment: 'UAT', + objects: ['provar__Person__c'], + field_filter: 'required', + }); + + const body = parseText(result); + const objects = body['objects'] as Array<{ required_fields: Array<{ name: string }>; field_count: number }>; + assert.equal(objects[0].field_count, 3, 'field_count reports all fields, not the filtered subset'); + const names = objects[0].required_fields.map((f) => f.name); + assert.deepEqual(names, ['provar__Email__c'], 'only the required field passes field_filter=required'); + }); + + it('(k.3) falls back to an existing env (default) when requested env missing', () => { + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + // Only `default` has cached metadata; the request asks for `UAT`. + writeSfObjectCache(siblingWorkspace, 'Admin', 'default', 'Account', 'Account', [ + { name: 'Name', type: 'string', required: true }, + ]); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'Admin', + environment: 'UAT', + objects: ['Account'], + field_filter: 'all', + }); + + assert.equal(isError(result), false); + const body = parseText(result); + const objects = body['objects'] as Array<{ name: string; exists: boolean | null; field_count: number }>; + assert.equal(objects[0].exists, true, 'should fall back to the default environment cache'); + assert.equal(objects[0].field_count, 1); + }); + + it('(k.4) defaults environment to "default" when omitted', () => { + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + writeSfObjectCache(siblingWorkspace, 'Admin', 'default', 'Account', 'Account', [ + { name: 'Name', type: 'string', required: true }, + ]); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'Admin', + objects: ['Account'], + }); + + assert.equal(isError(result), false); + const body = parseText(result); + const objects = body['objects'] as Array<{ name: string; exists: boolean | null }>; + assert.equal(objects[0].exists, true); + }); + + it('(k.5) treats a stub (detailsLoaded=false, no ) as exists=true, field_count=0', () => { + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + writeSfObjectCache(siblingWorkspace, 'Admin', 'default', 'Account', 'Account', [], false); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'Admin', + objects: ['Account'], + field_filter: 'all', + }); + + assert.equal(isError(result), false); + const body = parseText(result); + const objects = body['objects'] as Array<{ + name: string; + exists: boolean | null; + field_count: number; + error_message?: string; + }>; + assert.equal(objects[0].exists, true, 'stub file exists'); + assert.equal(objects[0].field_count, 0, 'stub has no loaded fields'); + assert.equal(objects[0].error_message, undefined, 'a stub is not a parse error'); + }); + + it('(k.6) lists all cached objects in the SfObject dir when objects omitted', () => { + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + writeSfObjectCache(siblingWorkspace, 'Admin', 'default', 'Account', 'Account', [{ name: 'Name', type: 'string' }]); + writeSfObjectCache(siblingWorkspace, 'Admin', 'default', 'Contact', 'Contact', [ + { name: 'LastName', type: 'string' }, + ]); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'Admin', + }); + + const body = parseText(result); + assert.ok(typeof body['cache_age_ms'] === 'number', 'cache_age_ms reflects the SfObject dir mtime'); + const objects = body['objects'] as Array<{ name: string }>; + // Display names come from sfObject @t; both objects use their own name here. + const names = objects.map((o) => o.name).sort(); + assert.deepEqual(names, ['Account', 'Contact']); + }); + + it('(k.7) prefers the IDE SfObject layout over the legacy .metadata/ layout', () => { + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + // Legacy layout has 1 field; IDE layout has 2. IDE must win. + writeJsonCache(path.join(siblingWorkspace, '.metadata', 'Admin'), 'Account', [ + { name: 'LegacyOnly', type: 'string', defaultValue: null, nillable: false }, + ]); + writeSfObjectCache(siblingWorkspace, 'Admin', 'default', 'Account', 'Account', [ + { name: 'Name', type: 'string', required: true }, + { name: 'Phone', type: 'phone' }, + ]); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'Admin', + objects: ['Account'], + field_filter: 'all', + }); + + const body = parseText(result); + const objects = body['objects'] as Array<{ field_count: number; required_fields: Array<{ name: string }> }>; + assert.equal(objects[0].field_count, 2, 'IDE SfObject layout should be preferred over legacy'); + const names = objects[0].required_fields.map((f) => f.name).sort(); + assert.deepEqual(names, ['Name', 'Phone']); + }); + + it('(k.8) rejects an environment containing a path separator with PATH_TRAVERSAL', () => { + const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); + writeSfObjectCache(siblingWorkspace, 'Admin', 'default', 'Account', 'Account', [{ name: 'Name', type: 'string' }]); + + const result = server.call('provar_org_describe', { + project_path: projectPath, + connection_name: 'Admin', + environment: '../../escape', + objects: ['Account'], + }); + + assert.equal(isError(result), true); + const code = parseText(result)['error_code'] as string; + assert.ok(code === 'PATH_TRAVERSAL' || code === 'PATH_NOT_ALLOWED', `Unexpected error_code: ${code}`); + }); + + it('(k.9) rejects a workspace SfObject path outside allowedPaths', () => { + // Build a workspace OUTSIDE the allowed root and point project discovery at it via a + // sibling whose parent is outside tmpRoot. The IDE-layout SfObject dir then sits + // outside allowedPaths and must be rejected by the path policy. + const outsideRoot = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'org-describe-out-'))); + try { + const outsideProject = path.join(outsideRoot, 'OutProject'); + fs.mkdirSync(outsideProject, { recursive: true }); + const outsideWorkspace = path.join(outsideRoot, 'workspace-OutProject'); + writeSfObjectCache(outsideWorkspace, 'Admin', 'default', 'Account', 'Account', [ + { name: 'Name', type: 'string' }, + ]); + + // Server allows ONLY tmpRoot, but project_path/workspace live under outsideRoot. + const result = server.call('provar_org_describe', { + project_path: outsideProject, + connection_name: 'Admin', + objects: ['Account'], + }); + + assert.equal(isError(result), true, 'a workspace outside allowedPaths must be rejected'); + const code = parseText(result)['error_code'] as string; + assert.ok(code === 'PATH_NOT_ALLOWED' || code === 'PATH_TRAVERSAL', `Unexpected error_code: ${code}`); + } finally { + fs.rmSync(outsideRoot, { recursive: true, force: true }); + } + }); +});