From a15e3739f893200cd21fe1960ffaf2959caf3d23 Mon Sep 17 00:00:00 2001 From: Tarun Prashar Date: Wed, 1 Jul 2026 19:01:45 +0530 Subject: [PATCH] PDX-0: refactor(mcp): change org_describe workspace discovery paths RCA: org_describe resolved the Provar workspace via a sibling workspace- dir; the layout needed updating so the project's parent directory is treated as the workspace root. Fix: workspaceCandidates now yields /, /Provar_Workspaces/workspace-/, and ~/Provar/workspace-/; discoverWorkspace requires a .metadata dir so candidate 1 falls through when the parent is not a workspace; removed the now-unused projectNameDashes helper and updated tests and docs. --- docs/mcp.md | 10 ++--- src/mcp/tools/orgDescribeTools.ts | 44 +++++++++---------- test/unit/mcp/orgDescribeTools.test.ts | 59 +++++++++++--------------- 3 files changed, 49 insertions(+), 64 deletions(-) diff --git a/docs/mcp.md b/docs/mcp.md index a3798c4..5f377da 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -2038,13 +2038,13 @@ Read cached Salesforce describe data for one connection from the Provar workspac **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: +**Workspace discovery heuristic** — the tool walks candidate directories in this order and uses the first one that is a Provar workspace (i.e. contains a `.metadata` directory): -1. `/workspace-/` — sibling workspace pattern (default for Provar IDE in this workspace layout). -2. `/Provar_Workspaces/workspace-/` — shared `Provar_Workspaces` directory. -3. `~/Provar/workspace-/` — user-home fallback. +1. `/` — the project's parent directory (the project lives inside its workspace; default for the Provar IDE layout). +2. `/Provar_Workspaces/workspace-/` — shared `Provar_Workspaces` directory. +3. `~/Provar/workspace-/` — user-home fallback. -`` is the project's basename with whitespace collapsed to single dashes and lowercased: `"My Project"` → `"my-project"`. +`` is the project directory's name verbatim (e.g. `"MyProject"`). A candidate is skipped unless it contains a `.metadata` directory, so candidate 1 (which almost always exists) falls through to the fallbacks when the parent is not itself a workspace. Each candidate is also checked against `--allowed-paths` before any filesystem access; candidates outside the policy are silently skipped. **Cache layout resolution (within the discovered workspace).** The tool prefers the Provar IDE SfObject cache and falls back to the legacy/native layout: diff --git a/src/mcp/tools/orgDescribeTools.ts b/src/mcp/tools/orgDescribeTools.ts index 1184c87..cec2ff9 100644 --- a/src/mcp/tools/orgDescribeTools.ts +++ b/src/mcp/tools/orgDescribeTools.ts @@ -78,44 +78,42 @@ interface CachedObject { // ── Workspace discovery ─────────────────────────────────────────────────────── -/** - * Normalise a project basename for use in fallback workspace dir names: - * "My Project Path " → "my-project-path". - */ -export function projectNameDashes(projectPath: string): string { - return path.basename(projectPath).trim().replace(/\s+/g, '-').toLowerCase(); -} - /** * Build the ordered list of candidate workspace directories. * First existing wins. - * 1. /workspace-/ — sibling workspace pattern. - * 2. /Provar_Workspaces/workspace-/ - * 3. ~/Provar/workspace-/ — user-home fallback. + * 1. / — the project's parent directory. + * 2. /Provar_Workspaces/workspace-/ + * 3. ~/Provar/workspace-/ — user-home fallback. */ export function workspaceCandidates(projectPath: string): string[] { const resolved = path.resolve(projectPath); const parent = path.dirname(resolved); const basename = path.basename(resolved); - const dashes = projectNameDashes(resolved); return [ - path.join(parent, `workspace-${basename}`), - path.join(parent, 'Provar_Workspaces', `workspace-${dashes}`), - path.join(os.homedir(), 'Provar', `workspace-${dashes}`), + parent, + path.join(parent, 'Provar_Workspaces', `workspace-${basename}`), + path.join(os.homedir(), 'Provar', `workspace-${basename}`), ]; } /** - * Returns the first candidate workspace that exists AND is within allowedPaths, or null. + * Returns the first candidate that is a real Provar workspace AND is within + * allowedPaths, or null. + * + * A candidate qualifies only if it is a directory that contains a `.metadata` + * subdirectory — the marker of a Provar/Eclipse workspace. This matters now that + * candidate 1 is the project's parent directory, which almost always exists: the + * `.metadata` requirement lets discovery fall through to the Provar_Workspaces and + * ~/Provar fallbacks when the parent is not itself a workspace. * * Path policy is enforced PER CANDIDATE before any filesystem call: a candidate that * sits outside `--allowed-paths` is silently skipped (it is not an error — discovery * just moves on to the next). This means we never call fs.existsSync / fs.statSync * against directories that the operator has explicitly placed off-limits, including - * the user-home fallback (~/Provar/...) when home sits outside the policy. + * the user-home fallback (~/Provar/workspace-/) when home sits outside the policy. * * When allowedPaths is empty (unrestricted mode), assertPathAllowed is a no-op and - * all candidates are probed exactly as before. + * all candidates are probed. */ export function discoverWorkspace(projectPath: string, allowedPaths: string[] = []): string | null { for (const candidate of workspaceCandidates(projectPath)) { @@ -125,12 +123,10 @@ export function discoverWorkspace(projectPath: string, allowedPaths: string[] = // Candidate outside policy — skip without touching the filesystem. continue; } - try { - if (fs.existsSync(candidate) && fs.statSync(candidate).isDirectory()) { - return candidate; - } - } catch { - // Permission errors etc. — skip and try next candidate + // A candidate qualifies only if it holds a `.metadata` subdirectory; this + // also implies the candidate itself is an existing directory. + if (isExistingDir(path.join(candidate, '.metadata'))) { + return candidate; } } return null; diff --git a/test/unit/mcp/orgDescribeTools.test.ts b/test/unit/mcp/orgDescribeTools.test.ts index 2ac588f..ab35130 100644 --- a/test/unit/mcp/orgDescribeTools.test.ts +++ b/test/unit/mcp/orgDescribeTools.test.ts @@ -14,7 +14,6 @@ import { describe, it, beforeEach, afterEach } from 'mocha'; import { registerOrgDescribe, discoverWorkspace, - projectNameDashes, workspaceCandidates, } from '../../../src/mcp/tools/orgDescribeTools.js'; import type { ServerConfig } from '../../../src/mcp/server.js'; @@ -163,11 +162,14 @@ beforeEach(() => { // Use realpathSync to canonicalise the path on macOS (/var → /private/var) so // assertPathAllowed comparisons match the realpath the policy resolves to. tmpRoot = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'org-describe-test-'))); - projectPath = path.join(tmpRoot, 'MyProject'); + // The project lives INSIDE its workspace directory; discovery resolves the workspace + // as the project's parent (/). Naming the parent workspace-MyProject lets the + // per-test fixtures keep writing caches under /workspace-MyProject. + projectPath = path.join(tmpRoot, 'workspace-MyProject', 'MyProject'); fs.mkdirSync(projectPath, { recursive: true }); server = new MockMcpServer(); - // tmpRoot must be allowed so both the project path and any sibling workspace + // tmpRoot must be allowed so both the project path and any workspace // candidate (also under tmpRoot) pass the path policy check. config = { allowedPaths: [tmpRoot] }; registerOrgDescribe(server as never, config); @@ -177,38 +179,28 @@ afterEach(() => { fs.rmSync(tmpRoot, { recursive: true, force: true }); }); -// ── projectNameDashes / workspaceCandidates ─────────────────────────────────── - -describe('projectNameDashes', () => { - it('lowercases and replaces whitespace with single dashes', () => { - assert.equal(projectNameDashes('/x/My Project Path'), 'my-project-path'); - assert.equal(projectNameDashes('/x/ Spaced Name '), 'spaced-name'); - }); -}); +// ── workspaceCandidates ─────────────────────────────────────────────────────── describe('workspaceCandidates', () => { it('returns three candidates in expected order', () => { const cands = workspaceCandidates('/Users/alice/projects/My Project'); assert.equal(cands.length, 3); + assert.equal(cands[0], path.resolve('/Users/alice/projects'), `Expected project parent first, got: ${cands[0]}`); assert.ok( - cands[0].endsWith(`${path.sep}workspace-My Project`), - `Expected sibling workspace first, got: ${cands[0]}` - ); - assert.ok( - cands[1].includes('Provar_Workspaces') && cands[1].endsWith('workspace-my-project'), + cands[1].includes('Provar_Workspaces') && cands[1].endsWith('workspace-My Project'), `Expected Provar_Workspaces second, got: ${cands[1]}` ); - assert.ok(cands[2].endsWith(`${path.sep}Provar${path.sep}workspace-my-project`)); + assert.ok(cands[2].endsWith(`${path.sep}Provar${path.sep}workspace-My Project`)); }); }); -// ── (a) Workspace discovery — sibling pattern ───────────────────────────────── +// ── (a) Workspace discovery — project parent ────────────────────────────────── describe('provar_org_describe — workspace discovery', () => { - it('(a) finds the sibling workspace at /workspace-', () => { - // /workspace-MyProject is the sibling pattern - const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); - const connectionDir = path.join(siblingWorkspace, '.metadata', 'MyOrg'); + it('(a) finds the workspace at the project parent (/)', () => { + // The project's parent dir (/workspace-MyProject) is the workspace. + const workspace = path.join(tmpRoot, 'workspace-MyProject'); + const connectionDir = path.join(workspace, '.metadata', 'MyOrg'); writeJsonCache(connectionDir, 'Account', [{ name: 'Name', type: 'string', defaultValue: null, nillable: false }]); const result = server.call('provar_org_describe', { @@ -218,7 +210,7 @@ describe('provar_org_describe — workspace discovery', () => { assert.equal(isError(result), false); const body = parseText(result); - assert.equal(body['workspace_path'], siblingWorkspace, 'should discover sibling workspace'); + assert.equal(body['workspace_path'], workspace, 'should discover the project-parent workspace'); const objects = body['objects'] as Array<{ name: string; exists: boolean | null; field_count: number }>; assert.equal(objects.length, 1); assert.equal(objects[0].name, 'Account'); @@ -226,14 +218,14 @@ describe('provar_org_describe — workspace discovery', () => { assert.equal(objects[0].field_count, 1); }); - it('(b) falls back to user-home workspace when sibling missing (via override)', () => { + it('(b) falls back to user-home workspace when project parent is not a workspace (via override)', () => { // Stand in for ~/Provar by using a HOME override. The tool uses os.homedir(), // and we override $HOME for this test only. Set the home to a tmp dir so the // path is inside allowed paths. const fakeHome = path.join(tmpRoot, 'fakehome'); fs.mkdirSync(fakeHome, { recursive: true }); - const homeWorkspace = path.join(fakeHome, 'Provar', 'workspace-myproject'); + const homeWorkspace = path.join(fakeHome, 'Provar', 'workspace-MyProject'); const connectionDir = path.join(homeWorkspace, '.metadata', 'MyOrg'); writeJsonCache(connectionDir, 'Contact', [ { name: 'LastName', type: 'string', defaultValue: null, nillable: false }, @@ -270,20 +262,17 @@ describe('provar_org_describe — workspace discovery', () => { }); it('discoverWorkspace skips candidates outside allowedPaths without touching the filesystem', () => { - // Create a sibling workspace that DOES exist on disk but lies outside the allowed root. - // We force this by creating a parallel tmp tree and only allowing tmpRoot. - // The sibling pattern resolves to /workspace-; the parent of - // projectPath is tmpRoot, so the sibling itself is inside the allowed set — - // which is what we want for the happy case. To exercise the policy skip we use - // a separate "outside" project path whose sibling candidate lives outside. + // Create a real workspace (with .metadata) that DOES exist on disk but lies outside + // the allowed root. The project parent (candidate 1) resolves to outsideRoot, so it + // would otherwise qualify — but it must be skipped because it is outside policy. const outsideRoot = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'org-describe-outside-'))); try { const outsideProject = path.join(outsideRoot, 'OtherProject'); fs.mkdirSync(outsideProject, { recursive: true }); - const outsideSibling = path.join(outsideRoot, 'workspace-OtherProject'); - fs.mkdirSync(outsideSibling, { recursive: true }); + // outsideRoot is the project parent (candidate 1); make it a genuine workspace. + fs.mkdirSync(path.join(outsideRoot, '.metadata'), { recursive: true }); - // With only tmpRoot allowed, discoverWorkspace MUST NOT return the outside sibling + // With only tmpRoot allowed, discoverWorkspace MUST NOT return the outside workspace // even though it exists on disk. assert.equal(discoverWorkspace(outsideProject, [tmpRoot]), null); } finally { @@ -296,7 +285,7 @@ describe('provar_org_describe — workspace discovery', () => { describe('provar_org_describe — cache miss', () => { it('(c) returns suggestion when workspace exists but .metadata/ absent', () => { - // Create the sibling workspace dir but NOT the connection subdir + // The project parent is the workspace; create its .metadata but NOT the connection subdir const siblingWorkspace = path.join(tmpRoot, 'workspace-MyProject'); fs.mkdirSync(path.join(siblingWorkspace, '.metadata'), { recursive: true });