Misc updates

[jamiew]

• Update @modelcontextprotocol/sdk to 1.20.1
• Add ToolAnnotations support to type definitions and factory
• Add annotations to core tools (run_glif, glif_info)
• Add readOnlyHint annotations to discovery tools
• Add destructiveHint annotations to metaskill tools
• Refactor tool registration with centralized registry

[claude]

Overall Assessment: ✅ APPROVED - Excellent refactoring with modern MCP features

This PR successfully modernizes the codebase with MCP SDK 1.20.1 and introduces a clean registry pattern. The changes are well-structured and improve maintainability.

View full review

---

Code Quality and Best Practices ⭐

Strengths:

1. Excellent refactoring - The new registry.ts centralizes tool registration and eliminates duplication from src/tools/index.ts:41-88
2. Clear separation of concerns - Tool groups are now self-documenting with their enable conditions
3. Consistent TypeScript patterns - Good use of interfaces (ToolGroupConfig in src/tools/registry.ts:29-33)
4. Well-documented code - JSDoc comments on the registry explain the purpose clearly

Suggestions:

1. Add export keyword - The ToolConfig interface in src/utils/tool-factory.ts:10 should be exported for external use, especially since annotations was added
2. Consider readonly arrays - TOOL_REGISTRY could use readonly to prevent accidental mutations:

   export const TOOL_REGISTRY: readonly ToolGroupConfig[] = [

---

Potential Bugs or Issues 🐛

Minor concerns:

1. Missing listSavedGlifTools import - In src/tools/registry.ts:10, listSavedGlifTools is imported but the tool is defined in the metaskill group (line 64), not discovery group where it semantically belongs. Consider if this tool should be in discovery instead.

2. Conditional spreading - The pattern in src/utils/tool-factory.ts:48:

   ...(config.annotations && { annotations: config.annotations }),

   is good, but should also handle empty objects. Consider:

   ...(config.annotations && Object.keys(config.annotations).length > 0 && { annotations: config.annotations }),

3. Type safety - The getEnabledTools() function could benefit from a return type assertion to ensure all tools have both definition and handler properties.

---

Performance Considerations ⚡

Good:

1. No performance regressions - The registry pattern is just as efficient as the previous approach
2. Lazy evaluation - The enabled() functions are called at runtime, allowing dynamic configuration

Potential improvements:

1. Cache enabled tools - If getEnabledTools() is called frequently, consider memoizing the result:

   let cachedTools: ToolGroup | null = null;
   export function getEnabledTools(): ToolGroup {
     if (cachedTools) return cachedTools;
     // ... existing logic
     cachedTools = allTools;
     return allTools;
   }

   However, this may not be needed if environment variables don't change at runtime.

---

Security Concerns 🔒

Good practices:

1. No security regressions - The refactoring doesn't introduce new attack vectors
2. Proper annotations - destructiveHint: true on remove_all_glif_tools and remove_glif_tool is excellent for user protection
3. Read-only hints - Discovery tools correctly marked as readOnlyHint: true

Observations:

1. Annotation consistency - run_glif has destructiveHint: false but could potentially consume API credits. Consider if this should be documented differently or if a resourceConsumingHint would be useful (though not part of MCP spec).

---

Test Coverage 🧪

Concerns:

1. No tests for registry.ts - The new src/tools/registry.ts file has no corresponding test file
2. Test updates needed - src/tools/tools.test.ts:119 hardcodes expected tool counts. These tests may fail with the new registry structure.

Recommendations:

1. Add registry tests:

   // src/tools/registry.test.ts
   describe('Tool Registry', () => {
     it('should return all tools when all groups enabled', () => {
       // mock env.*.enabled() to return true
       const tools = getEnabledTools();
       expect(Object.keys(tools).length).toBe(expectedCount);
     });
     
     it('should exclude disabled groups', () => {
       // mock env.discovery.enabled() to return false
       const tools = getEnabledTools();
       expect(tools['list_featured_glifs']).toBeUndefined();
     });
   });

2. Update existing tests - The test in src/tools/tools.test.ts:119 expects 12 tools, but this should be validated against the new registry structure.

3. Test annotations - Add tests to verify that:

   • All tools have appropriate annotations
   • readOnlyHint is correctly set for discovery tools
   • destructiveHint is correctly set for metaskill removal tools

---

Additional Observations 📝

Documentation:

• No CLAUDE.md file exists (attempted to read it earlier), so no style guide violations can be checked
• README.md is comprehensive and up-to-date

Migration notes:

• The SDK upgrade from 1.17.5 → 1.20.1 includes ToolAnnotations support
• Several peer dependencies were marked in package-lock.json (zod, express, etc.) - this is expected with npm 7+

Type definition improvements:

The addition of ToolAnnotations to ToolDefinition in src/tools/index.ts:24 is well-integrated with the factory pattern in src/utils/tool-factory.ts:16.

---

Action Items (Optional):

1. ✅ High Priority: Add tests for registry.ts
2. ✅ High Priority: Update existing test expectations in tools.test.ts
3. ⚠️ Medium Priority: Export ToolConfig interface if needed externally
4. ⚠️ Medium Priority: Consider moving listSavedGlifTools to appropriate group
5. ℹ️ Low Priority: Add memoization if getEnabledTools() becomes a bottleneck
6. ℹ️ Low Priority: Make TOOL_REGISTRY readonly

---

Overall: This is a solid refactoring that improves code organization and adds modern MCP features. The main gap is test coverage for the new registry module. Great work! 🚀

[claude]

✅ Looks Good to Merge

This PR successfully updates the SDK and refactors the tool registration system with annotations support. The changes are well-structured and improve maintainability.

View full review

Code Quality & Best Practices

Strengths:

• Excellent refactoring: The new centralized registry pattern (src/tools/registry.ts:37-76) is a significant improvement over the previous scattered tool definitions
• Clear separation of concerns: Registry now owns all tool grouping logic, making it easier to understand what tools are available
• Type safety: Proper TypeScript types throughout, including the new ToolAnnotations type from the SDK
• Consistent patterns: All tools now follow the same structure with annotations support
• Good documentation: JSDoc comments on the registry interface (src/tools/registry.ts:24-30)

Minor Issues:

• Import organization: In registry.ts, bot tools are imported mid-file (line 6) between core and discovery tools. Consider grouping all imports by category at the top
• Comment placement: Discovery tools comment on line 8 could be clearer about what discovery means in this context

Potential Bugs or Issues

Low Risk:

1. Deprecated field in package-lock.json: Several packages now have peer: true added - this appears to be an npm artifact and shouldn't affect functionality, but verify these are actually peer dependencies
2. No validation for annotation values: Tools with readOnlyHint and destructiveHint don't validate these are used correctly

Edge Cases to Consider:

• What happens if env.discovery.enabled() throws an error? The registry doesn't handle exceptions in enable conditions
• If a tool name conflicts between groups, Object.assign will silently overwrite in getEnabledTools() (src/tools/registry.ts:86)

Performance Considerations

Positive:

• ✅ Registry uses lazy evaluation - tools are only gathered when getEnabledTools() is called
• ✅ No unnecessary loops or complex operations
• ✅ Reduced code duplication (62 lines removed net in index.ts)

Neutral:

• The registry creates a new object on each getEnabledTools() call, but this is called infrequently so impact is negligible

Security Concerns

Good Security Practices:

• ✅ Proper use of readOnlyHint on all discovery/info tools (glif-info, list-featured-glifs, my-glifs, etc.)
• ✅ Correct destructiveHint: true on dangerous operations (remove-all-glif-tools, remove-glif-tool)
• ✅ run_glif correctly marked as non-destructive (executes workflows but doesn't modify server state)
• ✅ SDK upgrade from 1.17.5 to 1.20.1 likely includes security fixes

Recommendations:

• Consider adding validation to ensure tools with remove or delete in the name have destructiveHint: true
• Document what destructiveHint means in this context (destroys saved state vs. makes API calls)

Test Coverage

Current State:

• ✅ Existing tests in tools.test.ts should cover the refactored code path
• ✅ Tests verify tool registration and saved glif functionality
• ⚠️ No tests for new registry.ts module - this is the main gap

Missing Test Coverage:

1. No unit tests for getEnabledTools() with different environment configurations
2. No tests verifying annotations are properly attached to tool definitions
3. No tests for tool name conflicts between groups
4. No tests that TOOL_REGISTRY structure is valid

Additional Observations

Documentation:

• README.md wasn't updated, but tool descriptions match existing documentation
• Tool titles in annotations are user-friendly (Run Glif, Get Glif Info)

SDK Update:

• Version bump from 1.17.5 to 1.20.1 is relatively minor
• The ToolAnnotations type was likely added in a version between these two

Annotations Summary:

• run_glif: readOnlyHint=false, destructiveHint=false ✅
• glif_info: readOnlyHint=true ✅
• Discovery tools: readOnlyHint=true ✅
• save_glif_as_tool: readOnlyHint=false, destructiveHint=false ✅
• remove_glif_tool: readOnlyHint=false, destructiveHint=true ✅
• remove_all_glif_tools: readOnlyHint=false, destructiveHint=true ✅

Recommendations

Before Merging:

1. Add basic unit tests for registry.ts to cover the refactoring
2. Run the existing test suite to ensure nothing broke (use npm test)
3. Consider organizing imports in registry.ts by category

Future Improvements:

1. Add validation to prevent tool name conflicts
2. Add error handling for enabled() functions
3. Consider caching getEnabledTools() result if called frequently
4. Document annotation semantics in a comment or README section

Overall Assessment: This is a solid refactoring that improves code organization and adds important annotations support. The main gap is test coverage for the new registry module. With tests added, this would be production-ready.