Add tool annotations and new manifest and MCPB/DXT build script #246
Conversation
WalkthroughAdds UI annotations to tool descriptors returned by ListTools, extends system info to include Node and process details and preserves systemInfo as a nested object in getConfig, introduces MCPB packaging artifacts and a build script, updates .gitignore, and fixes a README privacy link. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant NPM as npm
participant Script as build-mcpb.cjs
participant TSC as tsc
participant FS as FileSystem
participant MCPB as mcpb CLI
Dev->>NPM: npm run build:mcpb
NPM->>Script: start
Script->>TSC: tsc -p .
TSC-->>Script: dist/
Script->>FS: prepare mcpb-bundle/, load manifest.template.json
Script->>Script: inject {{VERSION}} from package.json
Script->>MCPB: mcpb validate manifest.json
MCPB-->>Script: validation result
Script->>FS: copy dist, package.json, README.md, LICENSE, PRIVACY.md, icon.png
Script->>NPM: install --production in bundle
NPM-->>Script: node_modules (prod)
Script->>MCPB: mcpb pack -> .mcpb
MCPB-->>Script: *.mcpb
Script-->>Dev: output bundle path / next steps
sequenceDiagram
autonumber
participant Caller as Caller
participant SYS as getSystemInfo
participant Node as detectNodeInfo
participant Proc as process
Caller->>SYS: getSystemInfo()
SYS->>Node: detectNodeInfo()
Node->>Proc: read process.version, execPath, env.npm_version
Proc-->>Node: node/process values
Node-->>SYS: nodeInfo {version, path, npmVersion?}
SYS->>Proc: read pid, arch, platform, versions
Proc-->>SYS: processInfo
SYS-->>Caller: SystemInfo { isDXT, nodeInfo?, processInfo, ... }
Note over Caller,SYS: getConfig() now embeds SystemInfo as nested object
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/server.ts (1)
309-319: Add annotations for consistency to remaining tools.For parity and predictable UI, consider annotating the tools below as well.
Apply diffs:
@@ { name: "create_directory", @@ inputSchema: zodToJsonSchema(CreateDirectoryArgsSchema), + annotations: { + title: "Create Directory", + readOnlyHint: false, + destructiveHint: true, + openWorldHint: false, + }, }, @@ { name: "start_search", @@ - inputSchema: zodToJsonSchema(StartSearchArgsSchema), + inputSchema: zodToJsonSchema(StartSearchArgsSchema), + annotations: { + title: "Start Search", + readOnlyHint: true + }, }, @@ { name: "stop_search", @@ - inputSchema: zodToJsonSchema(StopSearchArgsSchema), + inputSchema: zodToJsonSchema(StopSearchArgsSchema), + annotations: { + title: "Stop Search", + readOnlyHint: false + }, }, @@ { name: "give_feedback_to_desktop_commander", @@ - inputSchema: zodToJsonSchema(GiveFeedbackArgsSchema), + inputSchema: zodToJsonSchema(GiveFeedbackArgsSchema), + annotations: { + title: "Give Feedback", + readOnlyHint: true, + openWorldHint: true + }, }, @@ { name: "get_prompts", @@ - inputSchema: zodToJsonSchema(GetPromptsArgsSchema), + inputSchema: zodToJsonSchema(GetPromptsArgsSchema), + annotations: { + title: "Get Curated Prompts", + readOnlyHint: true + }, },Also applies to: 356-437, 469-482, 801-836, 838-870
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/server.ts(19 hunks)
🔇 Additional comments (1)
src/server.ts (1)
172-176: MCP permitsannotationson Tool objects — no change required.MCP Tool objects allow extra properties;
annotationsand the reserved_metamap are intended for UI/vendor metadata and official clients typically tolerate unknown fields. Keepannotationsfor UI hints; use_meta(namespaced keys) for vendor-specific metadata if needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.gitignore (2)
48-53: Good: MCP bundle artifacts and local registry tokens are ignoredThis prevents accidental commits of build outputs and secrets. Consider pairing this with repo-level secret scanning and a pre-commit hook (e.g., git-secrets) for defense in depth.
54-57: Future-proof the ad‑hoc test script ignores and ensure they don’t ship to npmIf these files are temporary utilities, ignore them via a simple glob so future siblings don’t get committed. Also, .gitignore doesn’t strictly control npm publishing—prefer a package.json "files" allowlist to keep dev/test files out of the published package.
Apply optionally:
# Test files test_files/ test_fix.js test_search.js +test_*.jsAnd in package.json (outside this hunk), consider:
{ "files": ["dist/", "src/**/*.d.ts", "README.md", "LICENSE"] }manifest.template.json (1)
7-10: Consider aligning author metadata consistency.The
authorfield uses "Desktop Commander Team" whilepackage.jsonline 7 shows "Eduards Ruzga". Consider whether this should be consistent across both files or if the team name is intentionally different for the bundle.scripts/build-mcpb.cjs (1)
80-87: Consider adding conditional file copying based on existence.While the script handles missing files gracefully with warning messages, it might be cleaner to define required vs optional files separately to improve maintainability and clarity.
// Step 5: Copy necessary files +const requiredFiles = [ + 'dist', + 'package.json', + 'README.md', + 'LICENSE' +]; + +const optionalFiles = [ + 'PRIVACY.md', + 'logo.png' +]; +// Copy required files (fail if missing) +requiredFiles.forEach(file => { + const srcPath = path.join(PROJECT_ROOT, file); + const destPath = path.join(BUNDLE_DIR, file); + + if (!fs.existsSync(srcPath)) { + console.error(`❌ Required file ${file} not found`); + process.exit(1); + } + + if (fs.statSync(srcPath).isDirectory()) { + fs.cpSync(srcPath, destPath, { recursive: true }); + } else { + fs.copyFileSync(srcPath, destPath); + } + console.log(`✅ Copied ${file}`); +}); + +// Copy optional files (warn if missing) +optionalFiles.forEach(file => { -const filesToCopy = [ - 'dist', - 'package.json', - 'README.md', - 'LICENSE', - 'PRIVACY.md', - 'logo.png' -]; - -filesToCopy.forEach(file => { const srcPath = path.join(PROJECT_ROOT, file); const destPath = path.join(BUNDLE_DIR, file); if (fs.existsSync(srcPath)) { if (fs.statSync(srcPath).isDirectory()) { - // Copy directory recursively fs.cpSync(srcPath, destPath, { recursive: true }); } else { - // Copy file fs.copyFileSync(srcPath, destPath); } console.log(`✅ Copied ${file}`); } else { console.log(`⚠️ Skipped ${file} (not found)`); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)README.md(1 hunks)manifest.future.json(1 hunks)manifest.template.json(1 hunks)package.json(1 hunks)scripts/build-mcpb.cjs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- manifest.future.json
- README.md
🔇 Additional comments (6)
.gitignore (1)
46-47: Confirm test artifact path in .gitignoreFile: .gitignore (lines 46–47) contains:
test/test_output/Verify the repository actually writes artifacts to test/test_output/ (vs. e.g. tests/output) and update .gitignore to the correct path to avoid accidental check-ins. Automated sandbox verification failed (missing tools); confirm locally.
package.json (1)
45-46: LGTM! New build scripts for MCPB bundling.The addition of
build:mcpbandbuild:mcpb:futurescripts provides a clean interface to generate MCPB bundles using the new build script, with appropriate parameterization for different manifest templates.manifest.template.json (2)
1-31: Well-structured manifest template with proper versioning.The manifest template follows MCPB conventions and includes all necessary fields for deployment. The
{{VERSION}}placeholder will be properly substituted by the build script.
14-22: Verify NODE_ENV production setting impact.The manifest explicitly sets
NODE_ENV=productionwhich may affect runtime behavior. Ensure this doesn't disable debugging capabilities or change functionality that users might expect during development or troubleshooting.Based on the search results, MCP servers commonly use environment variables for configuration, including NODE_ENV settings. The search shows that setting
NODE_ENV=productionin MCP configurations is a standard practice and typically used to control runtime behavior like logging levels and debugging features without breaking core functionality.scripts/build-mcpb.cjs (2)
1-171: Well-structured build script with comprehensive error handling.The script provides a complete MCPB bundle generation workflow with proper validation, error handling, and user guidance. The logic correctly handles manifest template selection and variable substitution.
47-51: Resolved — manifest.future.json is present and verified.
manifest.future.json exists in the repository and its contents were inspected; no action required for the --future flag.
| execSync(`mcpb validate "${MANIFEST_PATH}"`, { stdio: 'inherit' }); | ||
| console.log('✅ Manifest validation passed'); | ||
| } catch (error) { | ||
| console.error('❌ Manifest validation failed:', error.message); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Verify mcpb CLI tool availability.
The script depends on the mcpb CLI tool for validation and packing but doesn't check if it's installed. This could cause runtime failures during the build process.
🏁 Script executed:
#!/bin/bash
# Description: Check if mcpb CLI tool is available and working
# Check if mcpb command exists
if command -v mcpb >/dev/null 2>&1; then
echo "✅ mcpb CLI tool found"
mcpb --version 2>/dev/null || echo "mcpb version command failed"
else
echo "❌ mcpb CLI tool not found"
echo "This tool is required for bundle validation and packing"
echo "Install it with: npm install -g mcpb"
exit 1
fiLength of output: 118
Add availability check for mcpb CLI before execution.
scripts/build-mcpb.cjs:126-131
The script currently invokes mcpb without verifying it’s installed, leading to opaque failures. Insert a preflight check (e.g. command -v mcpb or spawnSync('mcpb',['--version'])) and emit a clear error with install instructions before calling mcpb validate.
🤖 Prompt for AI Agents
In scripts/build-mcpb.cjs around lines 126 to 131, the script calls `mcpb
validate` without checking that the `mcpb` CLI is installed; add a preflight
availability check (for example use child_process.spawnSync('mcpb',
['--version']) or run a shell `command -v mcpb`) prior to execSync, and if the
check fails log a clear error message that explains mcpb is not found and gives
install instructions (or a link) and exit with code 1; only proceed to
execSync(`mcpb validate ...`) when the check succeeds.
wonderwhy-er
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
icon.pngis excluded by!**/*.png
📒 Files selected for processing (5)
manifest.template.json(1 hunks)package.json(1 hunks)scripts/build-mcpb.cjs(1 hunks)src/tools/config.ts(1 hunks)src/utils/system-info.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (3)
scripts/build-mcpb.cjs (1)
16-18: Add the missingmcpbCLI availability check.This repeats the earlier review: we still call
mcpbwithout verifying it exists, so users just see “command not found” fromexecSync. Add a quick--versionprobe (orcommand -v) with a clear install hint before the validate/pack steps.-const { execSync } = require('child_process'); +const { execSync, spawnSync } = require('child_process'); @@ // Step 7: Validate manifest console.log('🔍 Validating manifest...'); + +const mcpbCheck = spawnSync('mcpb', ['--version'], { stdio: 'ignore' }); +if (mcpbCheck.error || mcpbCheck.status !== 0) { + console.error('❌ mcpb CLI not found. Install it with: npm install -g mcpb'); + process.exit(1); +} + try { execSync(`mcpb validate "${MANIFEST_PATH}"`, { stdio: 'inherit' });Also applies to: 135-149
src/utils/system-info.ts (1)
509-723: Nice addition of Node/process diagnostics.Capturing the active Node binary, npm version, and process metadata will make troubleshooting much easier, especially when the server runs inside containers.
src/tools/config.ts (1)
17-23: Thanks for keepingsystemInfonested.Returning the structure intact avoids churn for future fields and keeps the payload predictable for clients that parse the config blob.
| "mcp_config": { | ||
| "command": "node", | ||
| "args": [ | ||
| "${__dirname}/dist/index.js" | ||
| ], |
There was a problem hiding this comment.
Fix the MCP command args path (literal ${__dirname} won’t resolve).
mcp_config.args is passed verbatim to the client process runner, so "${__dirname}/dist/index.js" remains a literal string and the launch fails with “ENOENT: no such file or directory, open '${__dirname}/dist/index.js'”. Use a relative path instead so the bundle starts correctly.
Apply this diff:
- "${__dirname}/dist/index.js"
+ "./dist/index.js"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "mcp_config": { | |
| "command": "node", | |
| "args": [ | |
| "${__dirname}/dist/index.js" | |
| ], | |
| "mcp_config": { | |
| "command": "node", | |
| "args": [ | |
| "./dist/index.js" | |
| ], |
🤖 Prompt for AI Agents
In manifest.template.json around lines 24 to 28, the mcp_config.args entry uses
the literal string "${__dirname}/dist/index.js" which won’t be resolved at
runtime; replace that element with a relative path to the bundled entry (for
example "./dist/index.js" or "dist/index.js") so the node process can locate and
start the bundle correctly when invoked by the client process runner.
| if (fs.existsSync(BUNDLE_DIR)) { | ||
| fs.rmSync(BUNDLE_DIR, { recursive: true }); | ||
| }fs.mkdirSync(BUNDLE_DIR, { recursive: true }); | ||
|
|
There was a problem hiding this comment.
Fix bundle directory creation syntax error.
} and fs.mkdirSync are glued together (}fs), so the script dies with Unexpected identifier before doing any work. Split them so the bundle directory actually gets created.
if (fs.existsSync(BUNDLE_DIR)) {
fs.rmSync(BUNDLE_DIR, { recursive: true });
-}fs.mkdirSync(BUNDLE_DIR, { recursive: true });
+}
+fs.mkdirSync(BUNDLE_DIR, { recursive: true });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (fs.existsSync(BUNDLE_DIR)) { | |
| fs.rmSync(BUNDLE_DIR, { recursive: true }); | |
| }fs.mkdirSync(BUNDLE_DIR, { recursive: true }); | |
| if (fs.existsSync(BUNDLE_DIR)) { | |
| fs.rmSync(BUNDLE_DIR, { recursive: true }); | |
| } | |
| fs.mkdirSync(BUNDLE_DIR, { recursive: true }); |
🤖 Prompt for AI Agents
In scripts/build-mcpb.cjs around lines 35 to 38, there's a syntax error caused
by `}` and `fs.mkdirSync` being concatenated (`}fs`) which throws an Unexpected
identifier; separate the statements by placing a semicolon or newline after the
closing brace so the fs.mkdirSync(BUNDLE_DIR, { recursive: true }) call is a
standalone statement, ensuring the bundle directory is created when it does not
exist.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/utils/system-info.ts (3)
40-51: SystemInfo additions look good; verify exposing process internals is acceptableAdding isDXT, nodeInfo, and processInfo is useful. However, surfacing process.versions, pid, and node exec path in user-facing guidance can increase environment fingerprinting. Confirm this is intended for your threat model. If not, consider narrowing processInfo (e.g., only node and v8) or gating usage behind a debug flag.
395-417: Avoid shadowingpathimport; make npm version detection robust
- Local const named
pathshadows the importedpathmodule; rename to avoid confusion.process.env.npm_versionis rarely set. Prefer parsingnpm_config_user_agentas a fallback.Apply:
function detectNodeInfo(): SystemInfo['nodeInfo'] { try { // Get Node.js version from current process - const version = process.version.replace('v', ''); // Remove 'v' prefix + const version = process.version.startsWith('v') ? process.version.slice(1) : process.version; // Remove 'v' prefix if present // Get Node.js executable path from current process - const path = process.execPath; + const execPath = process.execPath; // Get npm version from environment if available - const npmVersion = process.env.npm_version; + const npmVersion = + process.env.npm_version || + (process.env.npm_config_user_agent?.match(/npm\/([\d.]+)/)?.[1]); return { version, - path, + path: execPath, ...(npmVersion && { npmVersion }) }; } catch (error) { return undefined; } }
679-725: Gate env dump behind a verbosity flag; confirm PID/path disclosure is desiredIncluding PID, arch, platform, and engine versions in guidance is helpful for debugging but noisy and potentially over-discloses internals. Consider making this opt-in.
Apply:
- // Add process environment info - const envInfo = ` + // Add process environment info + const envInfo = ` Current Process Environment: - Node: v${processInfo.versions.node} - V8: v${processInfo.versions.v8} - Architecture: ${processInfo.arch} - Platform: ${processInfo.platform} - Process ID: ${processInfo.pid}`; + const includeEnv = /^(1|true)$/i.test(String(process.env.MCP_VERBOSE_GUIDANCE ?? '')); + const envInfoSection = includeEnv ? `\n${envInfo}` : ''; @@ - return ` + return ` COMMON WINDOWS DEVELOPMENT TOOLS: - ${nodeGuidance} ... -${envInfo}`; +${envInfoSection}`; @@ - return ` + return ` COMMON MACOS DEVELOPMENT TOOLS: ... -${envInfo}`; +${envInfoSection}`; @@ - return ` + return ` COMMON LINUX DEVELOPMENT TOOLS: ... -${envInfo}`; +${envInfoSection}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
manifest.template.json(1 hunks)src/utils/system-info.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- manifest.template.json
🔇 Additional comments (1)
src/utils/system-info.ts (1)
510-520: LGTM: processInfo capture is accuratepid/arch/platform/versions are gathered correctly and typed as NodeJS.ProcessVersions.
| isDXT: !!process.env.MCP_DXT, | ||
| nodeInfo, | ||
| processInfo, | ||
| examplePaths |
There was a problem hiding this comment.
🧩 Analysis chain
Bug: isDXT treats "false" as true
!!process.env.MCP_DXT makes any non-empty string truthy (including "false"). Parse the env as a real boolean.
Apply:
- isDXT: !!process.env.MCP_DXT,
+ isDXT: /^(1|true|yes)$/i.test(String(process.env.MCP_DXT ?? '')),Optionally, introduce a small helper for other boolean envs to keep behavior consistent with MCP_CLIENT_DOCKER.
🏁 Script executed:
#!/bin/bash
rg -n 'MCP_CLIENT_DOCKER' -C3 --type=tsLength of output: 445
🏁 Script executed:
#!/bin/bash
rg -n 'process.env.MCP_DXT' -C3 --type=ts
rg -n 'MCP_DXT' -C3 --type=ts
rg -n 'isDXT' -C3 --type=tsLength of output: 3121
Parse MCP_DXT as boolean
At src/utils/system-info.ts:539, replace
- isDXT: !!process.env.MCP_DXT,
+ isDXT: /^(1|true|yes)$/i.test(process.env.MCP_DXT ?? ''),Extract a helper (e.g. const envBool = (v?: string) => /^(1|true|yes)$/i.test(v ?? '');) to standardize boolean-env parsing (apply to both MCP_DXT and MCP_CLIENT_DOCKER).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| isDXT: !!process.env.MCP_DXT, | |
| nodeInfo, | |
| processInfo, | |
| examplePaths | |
| isDXT: /^(1|true|yes)$/i.test(process.env.MCP_DXT ?? ''), | |
| nodeInfo, | |
| processInfo, | |
| examplePaths |
Summary by CodeRabbit
New Features
Documentation
Chores