Merge pull request #114 from ProvarTesting/fix/mcp-metadata-download-ux

fix(mcp): improve metadata download UX and secrets field descriptions

Author: mrdailey99

.gitignore

@@ -56,8 +56,7 @@ mochawesome-report
 .env.*.local
 
 # Claude
-.claude/skills
-CLAUDE.md
+.claude/
 
 # NitroX schema files — do not commit until IP/licensing confirmed with Provar team
 # See: src/mcp/tools/nitroXTools.ts and plan notes

CLAUDE.md

@@ -0,0 +1,94 @@
+# Claude Code — Project Instructions
+
+This file is read automatically by Claude Code at the start of every session. Follow these rules when working in this repo.
+
+---
+
+## Documentation Requirements
+
+**Every PR that adds, modifies, or removes an MCP tool must update documentation.** The full set of places to update:
+
+| What changed | Where to update |
+|---|---|
+| New MCP tool | `docs/mcp.md` (add tool entry with schema + example), `docs/mcp-pilot-guide.md` (add evaluation scenario if user-facing), `README.md` (update tool count if referenced) |
+| Modified tool description / parameters / errors | `docs/mcp.md` (update the relevant tool section) |
+| Removed tool | `docs/mcp.md`, `docs/mcp-pilot-guide.md`, `README.md` |
+| New error code or suggestion field | `docs/mcp.md` (Troubleshooting or Error Codes section) |
+| Security model change (path policy, licence, transport) | `docs/mcp.md` (Security Model section), `docs/mcp-pilot-guide.md` (Security Model section) |
+| New NitroX tool or schema rule | `docs/mcp.md` (NitroX section), `docs/mcp-pilot-guide.md` (Scenario 7) |
+| Smoke test count change | Update `TOTAL_EXPECTED` in `scripts/mcp-smoke.cjs` |
+
+**External / customer-facing docs** (`docs/provar-mcp-public-docs.md`, `docs/university-of-provar-mcp-course.md`) are maintained separately by the Provar team — flag changes that affect public-facing behaviour in your PR description so those can be updated manually.
+
+---
+
+## Test Coverage Requirements
+
+Every PR must include tests for new or changed behaviour:
+
+- **New MCP tool** → unit tests in `test/unit/mcp/<toolGroup>Tools.test.ts` covering: happy path, path policy rejection, missing required fields, and any tool-specific error codes
+- **Modified error handling** → at least one positive test (error path fires) and one negative test (does not fire for non-matching input)
+- **New validation rule** → test that the rule fires correctly and that a valid input passes
+- **Smoke test** → add an entry in `scripts/mcp-smoke.cjs` for each new tool; update `TOTAL_EXPECTED`
+
+Run before every commit:
+```sh
+yarn test:only          # unit tests — must all pass
+node scripts/mcp-smoke.cjs 2>/dev/null   # smoke — must show all PASS
+yarn compile            # TypeScript — must be clean
+```
+
+---
+
+## MCP Tool Authoring Standards
+
+### Tool description quality
+
+Every tool description must answer these questions for an AI agent reading it cold:
+
+1. **What does it do?** (one sentence)
+2. **What prerequisite tools must run first?** (e.g. `config.load` before `metadata.download`)
+3. **What are the correct flags / parameters?** Include a concrete example in the `flags` field description when flags are free-form
+4. **What does a failure mean?** If a known error pattern exists (e.g. `[DOWNLOAD_ERROR]` = auth failure), say so in the description or return a `details.suggestion`
+
+### Field descriptions
+
+- Fields that accept a **string key or password** must say "string value, NOT a file path" if there is any risk of confusion with a path
+- Fields that accept a **file path** must note if the path must be within `--allowed-paths`
+- Optional fields that have a dangerous default (e.g. overwriting existing files) must call that out
+
+### Error responses
+
+- Return `details: { suggestion: '...' }` when a known error pattern maps to a common root cause and there is an actionable fix
+- Never pass `details: {}` — omit `details` entirely when there is nothing extra to say (keeps the response shape stable)
+- Error codes follow `SCREAMING_SNAKE_CASE`; document new codes in `docs/mcp.md`
+
+### Path safety
+
+- Call `assertPathAllowed(path, config.allowedPaths)` on **every** path input before any file operation — not just the output path
+- Use `path.resolve()` before `fs.existsSync` / `fs.readFileSync` / `fs.writeFileSync`
+- Never construct shell commands from user input; use `spawnSync` with an args array
+
+---
+
+## Branch and PR Conventions
+
+- Feature branches off `develop`: `feature/<description>`
+- Bug/fix branches off `develop`: `fix/<description>`
+- Release branches off `develop`: `release-v<semver>`
+- PRs target `develop`; releases are merged develop → main
+- Version in `package.json` follows `<major>.<minor>.<patch>-beta.<n>` on develop
+- Bump the beta suffix (`beta.N → beta.N+1`) on any PR that triggers a publish
+
+---
+
+## Lint
+
+The project uses ESLint with `@typescript-eslint` strict rules. Common gotchas:
+
+- `complexity` max is **20** — extract helpers if a function grows past that
+- `no-unsafe-assignment` / `no-unsafe-call` — cast through `unknown` not `any`
+- `no-unnecessary-type-assertion` — TypeScript narrows after `typeof x === 'string'` checks; remove the redundant cast
+- `camelcase` — `nitroX` is valid camelCase (capital X starts the next word)
+
+CI runs lint as part of `sf-prepack` — do not skip with `--no-verify` on the final merge commit.

src/mcp/tools/antTools.ts

@@ -192,7 +192,7 @@ export function registerAntGenerate(server: McpServer, config: ServerConfig): vo
         .string()
         .default('${env.ProvarSecretsPassword}')
         .describe(
-          'Password for the Provar secrets store. Defaults to reading from the ProvarSecretsPassword environment variable.'
+          'Encryption key used to decrypt the Provar .secrets file (the password string itself, not a file path). Defaults to reading from the ProvarSecretsPassword environment variable.'
         ),
       test_environment_secrets_password: z
         .string()

src/mcp/tools/automationTools.ts

@@ -233,12 +233,29 @@ export function registerAutomationCompile(server: McpServer): void {
 
 // ── Tool: provar.automation.metadata.download ─────────────────────────────────
 
+const DOWNLOAD_ERROR_SUGGESTION =
+  'A [DOWNLOAD_ERROR] almost always means a Salesforce authentication failure for the connection being used. ' +
+  'Check: (1) the connection credentials in the Provar project .secrets file are current and not expired; ' +
+  '(2) the named connection exists in the project and the name is spelled correctly (case-sensitive); ' +
+  '(3) if using a scratch org, confirm it has not expired (`sf org list`); ' +
+  '(4) if testprojectSecrets is set in provardx-properties.json, it must be the encryption key string used to decrypt .secrets — not a file path.';
+
 export function registerAutomationMetadataDownload(server: McpServer): void {
   server.tool(
     'provar.automation.metadata.download',
-    'Download Salesforce metadata into a Provar project. Invokes `sf provar automation metadata download`.',
+    [
+      'Download Salesforce metadata for one or more connections into a Provar project.',
+      'Invokes `sf provar automation metadata download`.',
+      'PREREQUISITE: Call provar.automation.config.load first — without it the command fails with MISSING_FILE.',
+      'Use the -c flag to specify connections: flags: ["-c", "ConnectionName1,ConnectionName2"].',
+      'Connection names are case-sensitive and must match the names defined in the Provar project.',
+      'If the download fails with [DOWNLOAD_ERROR], this is almost always a Salesforce authentication issue —',
+      'check that the credentials in the project .secrets file are current and that any referenced scratch orgs have not expired.',
+    ].join(' '),
     {
-      flags: z.array(z.string()).optional().default([]).describe('Raw CLI flags to forward (e.g. ["--target-org", "myorg"])'),
+      flags: z.array(z.string()).optional().default([]).describe(
+        'Raw CLI flags to forward. Use ["-c", "Name1,Name2"] (or the equivalent --connections form) to target specific connections. Example: ["-c", "MyOrg,SandboxOrg"]'
+      ),
       sf_path: z.string().optional().describe('Path to the sf CLI executable when not in PATH (e.g. "~/.nvm/versions/node/v22.0.0/bin/sf")'),
     },
     ({ flags, sf_path }) => {
@@ -247,12 +264,17 @@ export function registerAutomationMetadataDownload(server: McpServer): void {
 
       try {
         const result = runSfCommand(['provar', 'automation', 'metadata', 'download', ...flags], sf_path);
-        const response = { requestId, exitCode: result.exitCode, stdout: result.stdout, stderr: result.stderr };
+        const message = result.stderr || result.stdout;
 
         if (result.exitCode !== 0) {
-          return { isError: true as const, content: [{ type: 'text' as const, text: JSON.stringify(makeError('AUTOMATION_METADATA_FAILED', result.stderr || result.stdout, requestId)) }] };
+          const isDownloadError = message.includes('[DOWNLOAD_ERROR]');
+          const details: Record<string, unknown> | undefined = isDownloadError
+            ? { suggestion: DOWNLOAD_ERROR_SUGGESTION }
+            : undefined;
+          return { isError: true as const, content: [{ type: 'text' as const, text: JSON.stringify(makeError('AUTOMATION_METADATA_FAILED', message, requestId, false, details)) }] };
         }
 
+        const response = { requestId, exitCode: result.exitCode, stdout: result.stdout, stderr: result.stderr };
         return { content: [{ type: 'text' as const, text: JSON.stringify(response) }], structuredContent: response };
       } catch (err) {
         return handleSpawnError(err, requestId, 'provar.automation.metadata.download');

src/mcp/tools/propertiesTools.ts

@@ -224,7 +224,10 @@ const updatesSchema = z.object({
   pluginOutputlevel: z.enum(['SEVERE', 'WARNING', 'INFO', 'FINE', 'FINER', 'FINEST']).optional().describe('Amount of plugin output logged'),
   stopOnError: z.boolean().optional().describe('Abort test run on first failure'),
   excludeCallable: z.boolean().optional().describe('Omit callable test cases from execution'),
-  testprojectSecrets: z.string().optional().describe('Test project secrets encryption password'),
+  testprojectSecrets: z.string().optional().describe(
+    'Encryption key (password string) used to decrypt the .secrets file in the Provar project root. ' +
+    'This is the key itself — NOT a file path. Omit this field unless your project uses secrets encryption.'
+  ),
   environment: z.object({
     testEnvironment: z.string().optional().describe('Name of the test environment to run against'),
     webBrowser: z.enum(['Chrome', 'Safari', 'Edge', 'Edge_Legacy', 'Firefox', 'IE', 'Chrome_Headless']).optional(),

test/unit/mcp/automationTools.test.ts

@@ -422,6 +422,23 @@ describe('automationTools', () => {
       const result = server.call('provar.automation.metadata.download', { flags: [] });
       assert.equal(parseBody(result).error_code, 'SF_NOT_FOUND');
     });
+
+    it('includes suggestion in details when [DOWNLOAD_ERROR] is in the message', () => {
+      spawnStub.returns(makeSpawnResult('', 'Error (1): [DOWNLOAD_ERROR] ERROR\n', 1));
+      const result = server.call('provar.automation.metadata.download', { flags: ['-c', 'MyOrg'] });
+      assert.ok(isError(result));
+      const body = parseBody(result);
+      assert.equal(body.error_code, 'AUTOMATION_METADATA_FAILED');
+      assert.ok(body.details && typeof (body.details as Record<string, unknown>).suggestion === 'string', 'Expected suggestion in details for DOWNLOAD_ERROR');
+    });
+
+    it('does NOT include suggestion for other failure messages', () => {
+      spawnStub.returns(makeSpawnResult('', 'Error (2): Nonexistent flag: --properties-file\n', 1));
+      const result = server.call('provar.automation.metadata.download', { flags: [] });
+      assert.ok(isError(result));
+      const body = parseBody(result);
+      assert.ok(!body.details || !(body.details as Record<string, unknown>).suggestion, 'Expected no suggestion for non-DOWNLOAD_ERROR');
+    });
   });
 
   // ── provar.automation.config.load ─────────────────────────────────────────