diff --git a/.github/workflows/DeployManual.yml b/.github/workflows/DeployManual.yml index 0cfaf2e1..f8a31e0e 100644 --- a/.github/workflows/DeployManual.yml +++ b/.github/workflows/DeployManual.yml @@ -24,6 +24,13 @@ jobs: registry-url: 'https://registry.npmjs.org' scope: '@provartesting' - name: Install dependencies and build + env: + # Required by scripts/fetch-nitrox-packages.cjs to pull the latest + # NitroX schemas and component catalog from ProvarTesting/factPackages + # at release time. Without this, prepack falls back to the bundled + # snapshots and ships them as-is. The auto-provided GITHUB_TOKEN has + # read access to factPackages. + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | npm install -g @salesforce/cli yarn diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 75c17c0d..20429dba 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -81,17 +81,38 @@ export interface ServerConfig { export function parseActiveGroups(): Set | null { const env = process.env['PROVAR_MCP_TOOLS']; if (!env?.trim()) return null; - const groups = new Set( + const requested = new Set( env .split(',') .map((g) => g.trim().toLowerCase()) .filter(Boolean) ); - if (groups.size === 0) { + if (requested.size === 0) { log('warn', 'PROVAR_MCP_TOOLS was set but contained no valid group names — activating all groups', { raw: env }); return null; } - return groups; + const known = new Set(Object.keys(TOOL_GROUPS)); + const matched = new Set(); + const unknown: string[] = []; + for (const g of requested) { + if (known.has(g)) matched.add(g); + else unknown.push(g); + } + if (unknown.length > 0) { + log('warn', 'PROVAR_MCP_TOOLS contains unknown group names — they will be ignored', { + raw: env, + unknown, + known: [...known], + }); + } + if (matched.size === 0) { + log('warn', 'PROVAR_MCP_TOOLS matched no known group names — activating all groups', { + raw: env, + known: [...known], + }); + return null; + } + return matched; } export function createProvarMcpServer(config: ServerConfig): McpServer { diff --git a/src/mcp/tools/projectValidateFromPath.ts b/src/mcp/tools/projectValidateFromPath.ts index bae0ac29..a27c160a 100644 --- a/src/mcp/tools/projectValidateFromPath.ts +++ b/src/mcp/tools/projectValidateFromPath.ts @@ -23,6 +23,7 @@ import { hasAnyRun, loadBaselineViolations, computeDiff, + computeContextHash, type DiffableViolation, } from '../utils/validationDiff.js'; import { desc } from './descHelper.js'; @@ -297,6 +298,7 @@ export function registerProjectValidateFromPath(server: McpServer, config: Serve if (results_dir) assertPathAllowed(results_dir, config.allowedPaths); const storageDir = results_dir ?? path.join(project_path, 'provardx', 'validation'); + const contextHash = computeContextHash('project', project_path); const runId = generateRunId(project_path); const result = validateProjectFromPath({ @@ -318,14 +320,14 @@ export function registerProjectValidateFromPath(server: McpServer, config: Serve // Load baseline BEFORE saving to prevent eviction of the requested baseline. const baseline = baseline_run_id !== undefined && baseline_run_id !== '' - ? loadBaselineViolations(storageDir, baseline_run_id) + ? loadBaselineViolations(storageDir, baseline_run_id, contextHash) : null; const hasBaseline = hasAnyRun(storageDir); if (save_results !== false) { try { - saveRun(storageDir, runId, currentViolations); + saveRun(storageDir, runId, currentViolations, contextHash); } catch (saveErr) { log('warn', 'provar_project_validate: could not save run for diff', { requestId, diff --git a/src/mcp/tools/testCaseValidate.ts b/src/mcp/tools/testCaseValidate.ts index 0b163922..b374db21 100644 --- a/src/mcp/tools/testCaseValidate.ts +++ b/src/mcp/tools/testCaseValidate.ts @@ -7,7 +7,6 @@ /* eslint-disable camelcase */ import fs from 'node:fs'; -import os from 'node:os'; import path from 'node:path'; import { createHash } from 'node:crypto'; import { z } from 'zod'; @@ -33,6 +32,8 @@ import { hasAnyRun, loadBaselineViolations, computeDiff, + computeContextHash, + resolveValidationDir, type DiffableViolation, } from '../utils/validationDiff.js'; import { runBestPractices } from './bestPracticesEngine.js'; @@ -67,7 +68,7 @@ const TC_VALIDATE_SUMMARY_FIELDS = [ /** Storage dir for testcase diff runs (namespaced to avoid cross-tool baseline collisions). */ function tcStorageDir(): string { - return path.join(os.homedir(), '.provardx', 'validation', 'testcase'); + return resolveValidationDir('testcase'); } /** Resolve validation result from QualityHub API or fall back to local. */ @@ -183,7 +184,9 @@ export function registerTestCaseValidate(server: McpServer, config: ServerConfig const baseResult = await resolveBaseResult(source, apiKey, requestId); const storageDir = tcStorageDir(); - const runId = generateRunId(tcRunContext(file_path, source)); + const context = tcRunContext(file_path, source); + const contextHash = computeContextHash('tc', context); + const runId = generateRunId(context); const bpViolations = (baseResult.best_practices_violations ?? []) as unknown as DiffableViolation[]; const currentViolations: DiffableViolation[] = [ ...(baseResult.issues as unknown as DiffableViolation[]), @@ -193,13 +196,13 @@ export function registerTestCaseValidate(server: McpServer, config: ServerConfig // Load baseline BEFORE saving to prevent eviction of the requested baseline const baseline = baseline_run_id !== undefined && baseline_run_id !== '' - ? loadBaselineViolations(storageDir, baseline_run_id) + ? loadBaselineViolations(storageDir, baseline_run_id, contextHash) : null; const hasBaseline = hasAnyRun(storageDir); try { - saveRun(storageDir, runId, currentViolations); + saveRun(storageDir, runId, currentViolations, contextHash); } catch (saveErr) { log('warn', 'provar_testcase_validate: could not save run for diff', { requestId, diff --git a/src/mcp/tools/testSuiteValidate.ts b/src/mcp/tools/testSuiteValidate.ts index 585ae631..07e96c47 100644 --- a/src/mcp/tools/testSuiteValidate.ts +++ b/src/mcp/tools/testSuiteValidate.ts @@ -6,8 +6,6 @@ */ /* eslint-disable camelcase */ -import os from 'node:os'; -import path from 'node:path'; import { z } from 'zod'; import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import { makeError, makeRequestId } from '../schemas/common.js'; @@ -20,6 +18,8 @@ import { hasAnyRun, loadBaselineViolations, computeDiff, + computeContextHash, + resolveValidationDir, type DiffableViolation, } from '../utils/validationDiff.js'; import { validateSuite, buildHierarchySummary, type TestSuiteInput, type SuiteResult } from './hierarchyValidate.js'; @@ -78,7 +78,7 @@ const SUITE_VALIDATE_SUMMARY_FIELDS = [ ]; function suiteStorageDir(): string { - return path.join(os.homedir(), '.provardx', 'validation', 'testsuite'); + return resolveValidationDir('testsuite'); } export function registerTestSuiteValidate(server: McpServer): void { @@ -154,19 +154,20 @@ export function registerTestSuiteValidate(server: McpServer): void { const summary = buildHierarchySummary(result); const storageDir = suiteStorageDir(); + const contextHash = computeContextHash('suite', suite_name); const runId = generateRunId(suite_name); const currentViolations = collectAllViolations(result); // Load baseline BEFORE saving to prevent eviction of the requested baseline const baseline = baseline_run_id !== undefined && baseline_run_id !== '' - ? loadBaselineViolations(storageDir, baseline_run_id) + ? loadBaselineViolations(storageDir, baseline_run_id, contextHash) : null; const hasBaseline = hasAnyRun(storageDir); try { - saveRun(storageDir, runId, currentViolations); + saveRun(storageDir, runId, currentViolations, contextHash); } catch (saveErr) { log('warn', 'provar_testsuite_validate: could not save run for diff', { requestId, diff --git a/src/mcp/utils/validationDiff.ts b/src/mcp/utils/validationDiff.ts index a5c6a1ba..84106eae 100644 --- a/src/mcp/utils/validationDiff.ts +++ b/src/mcp/utils/validationDiff.ts @@ -7,11 +7,14 @@ /* eslint-disable camelcase */ import fs from 'node:fs'; +import os from 'node:os'; import path from 'node:path'; import { createHash } from 'node:crypto'; const MAX_RUNS = 20; const INDEX_FILE = '.runs.json'; +const DEFAULT_ROOT_NAME = '.provardx'; +const VALIDATION_SUBDIR = 'validation'; // ── Public types ────────────────────────────────────────────────────────────── @@ -28,6 +31,15 @@ interface RunRecord { run_id: string; timestamp: number; filename: string; + /** + * Hash of `${toolTag}|${context}`. Used by loadBaselineViolations to reject + * a run_id whose context (file path, suite name, etc.) does not match the + * calling context — prevents cross-context diffs. Optional for backward + * compatibility with index records written before this field existed; those + * older records are treated as not matching any caller and are effectively + * invalidated within one or two new runs as the FIFO cap evicts them. + */ + context_hash?: string; } interface RunsIndex { @@ -67,6 +79,27 @@ function saveIndex(storageDir: string, index: RunsIndex): void { // ── Public API ──────────────────────────────────────────────────────────────── +/** + * Compute a stable 8-char context hash for a tool + context pair. Used to + * scope baseline run lookups so that a run_id from context A cannot be diffed + * against context B (different project, different suite, different file). + */ +export function computeContextHash(toolTag: string, context: string): string { + return shortHash(`${toolTag}|${context}`); +} + +/** + * Resolve the validation storage root for a given tool subdir. Honors the + * PROVAR_MCP_VALIDATION_DIR env var when set; otherwise falls back to + * `~/.provardx/validation/`. The env override is useful for restricted + * CI/dev environments where the home directory is read-only or shared. + */ +export function resolveValidationDir(subdir: string): string { + const override = process.env['PROVAR_MCP_VALIDATION_DIR']?.trim(); + if (override) return path.join(override, subdir); + return path.join(os.homedir(), DEFAULT_ROOT_NAME, VALIDATION_SUBDIR, subdir); +} + /** Generate a run ID from a context string (e.g. project path or suite name). */ export function generateRunId(context: string): string { const rand = Math.random().toString(36).slice(2, 6); @@ -86,15 +119,29 @@ export function hasAnyRun(storageDir: string): boolean { * Save the current violations as a new run in the storage directory. * Caps the index at MAX_RUNS by evicting the oldest entry when full. * Returns the generated run_id. + * + * When `contextHash` is provided, it is recorded alongside the run so that + * `loadBaselineViolations` can reject a baseline_run_id whose context does + * not match the calling context (prevents cross-context diffs). */ -export function saveRun(storageDir: string, runId: string, violations: DiffableViolation[]): string { +export function saveRun( + storageDir: string, + runId: string, + violations: DiffableViolation[], + contextHash?: string +): string { fs.mkdirSync(storageDir, { recursive: true }); const filename = `${runId}.json`; fs.writeFileSync(path.join(storageDir, filename), JSON.stringify(violations), 'utf-8'); const index = loadIndex(storageDir); - index.runs.push({ run_id: runId, timestamp: Date.now(), filename }); + index.runs.push({ + run_id: runId, + timestamp: Date.now(), + filename, + ...(contextHash ? { context_hash: contextHash } : {}), + }); // Evict oldest entries when over the cap while (index.runs.length > MAX_RUNS) { @@ -117,12 +164,26 @@ export function saveRun(storageDir: string, runId: string, violations: DiffableV * Returns null if the run is not found in the index (BASELINE_NOT_FOUND). * The filename is looked up from the index only — the run_id itself is never * used to construct a file path, preventing path traversal. + * + * When `expectedContextHash` is provided, the record's `context_hash` must + * match. Records without a `context_hash` (written by older versions before + * H3) are treated as a mismatch and are effectively retired within one or + * two new runs as the FIFO cap evicts them. This guard prevents diffing a + * baseline from a different file/suite/project against the current context. */ -export function loadBaselineViolations(storageDir: string, baselineRunId: string): DiffableViolation[] | null { +export function loadBaselineViolations( + storageDir: string, + baselineRunId: string, + expectedContextHash?: string +): DiffableViolation[] | null { const index = loadIndex(storageDir); const record = index.runs.find((r) => r.run_id === baselineRunId); if (!record) return null; + if (expectedContextHash !== undefined && record.context_hash !== expectedContextHash) { + return null; + } + // Use the filename from the index, not the run_id try { const content = fs.readFileSync(path.join(storageDir, record.filename), 'utf-8'); diff --git a/test/unit/mcp/startupTuning.test.ts b/test/unit/mcp/startupTuning.test.ts index 3ef0550e..c16b532e 100644 --- a/test/unit/mcp/startupTuning.test.ts +++ b/test/unit/mcp/startupTuning.test.ts @@ -155,6 +155,25 @@ describe('parseActiveGroups() (PDX-469)', () => { process.env['PROVAR_MCP_TOOLS'] = ',,'; assert.equal(parseActiveGroups(), null); }); + + // ── H1: unknown group names ─────────────────────────────────────────────── + it('returns null when every requested group name is unknown (typo footgun)', () => { + // Pre-fix: a typo like 'validaton' produced Set{'validaton'} which matched + // no group and silently disabled all tools. Now we fall back to null (all + // tools) so the server is never left with an empty Provar tool surface. + process.env['PROVAR_MCP_TOOLS'] = 'validaton'; + assert.equal(parseActiveGroups(), null); + }); + + it('keeps known names and ignores unknown ones in a mixed list', () => { + process.env['PROVAR_MCP_TOOLS'] = 'nitroX,bogusgroup,validation'; + const groups = parseActiveGroups(); + assert.ok(groups instanceof Set); + assert.equal(groups.size, 2); + assert.ok(groups.has('nitrox')); + assert.ok(groups.has('validation')); + assert.ok(!groups.has('bogusgroup')); + }); }); // ── PDX-469: tool profile registration ──────────────────────────────────────── diff --git a/test/unit/mcp/testSuiteValidate.test.ts b/test/unit/mcp/testSuiteValidate.test.ts index 1a2f7817..8afeeb3e 100644 --- a/test/unit/mcp/testSuiteValidate.test.ts +++ b/test/unit/mcp/testSuiteValidate.test.ts @@ -7,7 +7,10 @@ /* eslint-disable camelcase */ import { strict as assert } from 'node:assert'; -import { describe, it, beforeEach } from 'mocha'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { describe, it, beforeEach, afterEach } from 'mocha'; import { registerTestSuiteValidate } from '../../../src/mcp/tools/testSuiteValidate.js'; // ── Minimal McpServer mock ───────────────────────────────────────────────────── @@ -70,18 +73,33 @@ const TC_LOGOUT = { name: 'LogoutTest.testcase', xml_content: makeXml(G.tc2, G.s const TC_LOGIN_ALIAS = { name: 'LoginTest.testcase', xml: makeXml(G.tc1, G.s1, 'tc-001') }; const TC_LOGOUT_ALIAS = { name: 'LogoutTest.testcase', xml: makeXml(G.tc2, G.s2, 'tc-002') }; -// ── Test setup ───────────────────────────────────────────────────────────────── - -let server: MockMcpServer; - -beforeEach(() => { - server = new MockMcpServer(); - registerTestSuiteValidate(server as never); -}); - // ── provar_testsuite_validate ───────────────────────────────────────────────── describe('provar_testsuite_validate', () => { + let server: MockMcpServer; + let origHomedir: () => string; + let tempHome: string; + + beforeEach(() => { + // Redirect os.homedir() into a temp dir so suiteStorageDir() writes to + // an isolated location instead of polluting the real developer/CI home. + // NOTE: scoped INSIDE this describe so the stub does not leak into other + // test files. Mocha root-level beforeEach attaches to the root suite and + // runs before every test in every file — see auth/rotate.test.ts which + // relies on the real os.homedir() and would otherwise see this stub. + tempHome = fs.mkdtempSync(path.join(os.tmpdir(), 'pvts-home-')); + origHomedir = os.homedir; + (os as unknown as { homedir: () => string }).homedir = (): string => tempHome; + + server = new MockMcpServer(); + registerTestSuiteValidate(server as never); + }); + + afterEach(() => { + (os as unknown as { homedir: () => string }).homedir = origHomedir; + fs.rmSync(tempHome, { recursive: true, force: true }); + }); + describe('happy path', () => { it('returns a result (not an error) for a valid non-empty suite', () => { const result = server.call('provar_testsuite_validate', { diff --git a/test/unit/mcp/validationDiff.test.ts b/test/unit/mcp/validationDiff.test.ts index 931a5ded..8560147f 100644 --- a/test/unit/mcp/validationDiff.test.ts +++ b/test/unit/mcp/validationDiff.test.ts @@ -10,6 +10,8 @@ import { hasAnyRun, loadBaselineViolations, computeDiff, + computeContextHash, + resolveValidationDir, } from '../../../src/mcp/utils/validationDiff.js'; const V1 = { rule_id: 'RULE-001', applies_to: 'TestSuite', message: 'Suite is empty' }; @@ -144,3 +146,86 @@ describe('computeDiff', () => { assert.equal(diff.unchanged_count, 1); }); }); + +// ── H3: cross-context scoping ───────────────────────────────────────────────── + +describe('computeContextHash', () => { + it('is deterministic for the same tool+context', () => { + assert.equal(computeContextHash('tc', '/a/b/c.testcase'), computeContextHash('tc', '/a/b/c.testcase')); + }); + + it('differs for different tools with the same context', () => { + assert.notEqual(computeContextHash('tc', '/path'), computeContextHash('suite', '/path')); + }); + + it('differs for different contexts under the same tool', () => { + assert.notEqual(computeContextHash('tc', '/a'), computeContextHash('tc', '/b')); + }); +}); + +describe('loadBaselineViolations — context scoping (H3)', () => { + it('returns null when expectedContextHash does not match the saved record', () => { + const ctxA = computeContextHash('tc', '/project/a/x.testcase'); + const ctxB = computeContextHash('tc', '/project/b/y.testcase'); + const runId = generateRunId('/project/a/x.testcase'); + saveRun(tmpDir, runId, [V1], ctxA); + + // Same store, same run_id, different context → should be rejected + assert.equal(loadBaselineViolations(tmpDir, runId, ctxB), null); + }); + + it('returns the violations when expectedContextHash matches', () => { + const ctx = computeContextHash('tc', '/project/a/x.testcase'); + const runId = generateRunId('/project/a/x.testcase'); + saveRun(tmpDir, runId, [V1, V2], ctx); + + const loaded = loadBaselineViolations(tmpDir, runId, ctx); + assert.deepEqual(loaded, [V1, V2]); + }); + + it('treats records written without context_hash as a mismatch when one is expected', () => { + // Simulate a record persisted by an older version (no context_hash) + const runId = generateRunId('/legacy/path'); + saveRun(tmpDir, runId, [V1]); // omit contextHash + const ctx = computeContextHash('tc', '/legacy/path'); + assert.equal(loadBaselineViolations(tmpDir, runId, ctx), null); + }); + + it('still loads when no expectedContextHash is provided (back-compat)', () => { + const runId = generateRunId('/path'); + saveRun(tmpDir, runId, [V1]); // no context hash + assert.deepEqual(loadBaselineViolations(tmpDir, runId), [V1]); + }); +}); + +describe('resolveValidationDir', () => { + let saved: string | undefined; + + beforeEach(() => { + saved = process.env['PROVAR_MCP_VALIDATION_DIR']; + delete process.env['PROVAR_MCP_VALIDATION_DIR']; + }); + + afterEach(() => { + if (saved !== undefined) process.env['PROVAR_MCP_VALIDATION_DIR'] = saved; + else delete process.env['PROVAR_MCP_VALIDATION_DIR']; + }); + + it('defaults to ~/.provardx/validation/ when env override is unset', () => { + const dir = resolveValidationDir('testcase'); + const expected = path.join(os.homedir(), '.provardx', 'validation', 'testcase'); + assert.equal(dir, expected); + }); + + it('honors PROVAR_MCP_VALIDATION_DIR when set', () => { + process.env['PROVAR_MCP_VALIDATION_DIR'] = path.join(tmpDir, 'custom-root'); + const dir = resolveValidationDir('testsuite'); + assert.equal(dir, path.join(tmpDir, 'custom-root', 'testsuite')); + }); + + it('trims whitespace and falls back to default when env is whitespace-only', () => { + process.env['PROVAR_MCP_VALIDATION_DIR'] = ' '; + const dir = resolveValidationDir('testcase'); + assert.equal(dir, path.join(os.homedir(), '.provardx', 'validation', 'testcase')); + }); +});