Add annotations #246
Add annotations #246
Conversation
WalkthroughAdds UI annotation metadata to tool descriptors returned by ListTools in Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client/UI
participant S as Server (`src/server.ts`)
UI->>S: ListTools request
Note over UI,S: Request available tools
S-->>UI: tools[] (each tool includes annotations { title, readOnlyHint?, destructiveHint?, openWorldHint? })
Note right of UI: UI uses annotations for presentation and constraints
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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.
Summary by CodeRabbit
New Features
Chores
Documentation