refactor(tools): Modularize tool system with self-registering plugins by rhblind · Pull Request #6 · rhblind/emacs-mcp-server

rhblind · 2026-01-08T01:10:26Z

Extract eval-elisp and get-diagnostics from mcp-server-emacs-tools.el into separate files in tools/ directory. Each tool now self-registers using the new declarative mcp-server-register-tool API.

Add mcp-server-register-tool function for declarative tool registration
Convert mcp-server-tools-register to legacy wrapper
Create tools/mcp-server-emacs-tools-eval-elisp.el (self-registering)
Create tools/mcp-server-emacs-tools-diagnostics.el (self-registering)
Update mcp-server-emacs-tools.el to load tool modules from tools/
Add comprehensive unit tests in test/unit/test-mcp-emacs-tools.el
Update documentation in CLAUDE.md and README.md with new architecture
Update package file patterns to include tools/*.el

Extract eval-elisp and get-diagnostics from mcp-server-emacs-tools.el into separate files in tools/ directory. Each tool now self-registers using the new declarative `mcp-server-register-tool` API. - Add mcp-server-register-tool function for declarative tool registration - Convert mcp-server-tools-register to legacy wrapper - Create tools/mcp-server-emacs-tools-eval-elisp.el (self-registering) - Create tools/mcp-server-emacs-tools-diagnostics.el (self-registering) - Update mcp-server-emacs-tools.el to load tool modules from tools/ - Add comprehensive unit tests in test/unit/test-mcp-emacs-tools.el - Update documentation in CLAUDE.md and README.md with new architecture - Update package file patterns to include tools/*.el

Copilot

Pull request overview

This PR refactors the MCP tool system to support a modular, self-registering plugin architecture. Tools are now extracted into separate files in a tools/ directory and use a new declarative registration API.

Key Changes:

Introduces mcp-server-register-tool as the preferred declarative API for tool registration
Extracts eval-elisp and get-diagnostics tools into separate self-registering modules
Updates the tools initialization to preserve pre-registered tools instead of clearing the registry

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
tools/mcp-server-emacs-tools-eval-elisp.el	New self-registering eval-elisp tool module
tools/mcp-server-emacs-tools-diagnostics.el	New self-registering get-diagnostics tool module with flycheck/flymake support
mcp-server-tools.el	Adds declarative `mcp-server-register-tool` API and converts legacy register function to wrapper
mcp-server-emacs-tools.el	Converted to tool loader that requires modules from tools/ directory
test/unit/test-mcp-emacs-tools.el	Comprehensive unit tests for new registration API and both tools
README.md	Updated documentation with new tool architecture and usage examples
CLAUDE.md	Updated development guide with new tool creation workflow

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

mcp-server-emacs-tools.el

test/unit/test-mcp-emacs-tools.el

tools/mcp-server-emacs-tools-diagnostics.el

tools/mcp-server-emacs-tools-eval-elisp.el

Add nil checks for load-file-name and buffer-file-name before using them to construct the tools directory path. This prevents errors when these variables are not set in certain loading contexts.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

mcp-server-emacs-tools.el

tools/mcp-server-emacs-tools-eval-elisp.el

Emphasize that eval-elisp enables arbitrary code execution and poses inherent security risks when granting LLMs access to evaluate Elisp.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

README.md

Introduce customizable tool selection to allow users to disable potentially dangerous tools like eval-elisp. Tools now register via explicit registration functions rather than on module load, enabling dynamic control through the new `mcp-server-emacs-tools-enabled` configuration option.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

README.md

CLAUDE.md

Replace manual tool registration with automatic self-registration on require. Tools now register themselves when loaded, and a new filtering system (`mcp-server-tools-filter`) controls which tools are exposed to clients at runtime. Changes: - Tools self-register on load instead of requiring explicit registration - Add `mcp-server-tools-filter` to enable/disable tools dynamically - Simplify `mcp-server-emacs-tools--available` to map names to features - Remove obsolete registration functions from tool modules - Update documentation to reflect new architecture - Add tests for tool filtering behavior

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

mcp-server-tools.el:260

When mcp-server-tools-cleanup is called (during server stop), it clears the tool registry with clrhash. However, the tool modules register themselves at load time using top-level code. On server restart, the modules are already loaded (via require), so they won't re-register themselves. This means after stopping and restarting the server, no tools will be available. Consider either: 1) Not clearing the registry in cleanup, or 2) Providing a re-registration mechanism that can be called on restart, or 3) Using load instead of require to force re-evaluation of the tool files.

(defun mcp-server-tools-cleanup ()
  "Clean up the tools system."
  (clrhash mcp-server-tools--registry)
  (setq mcp-server-tools--initialized nil))

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

test/unit/test-mcp-emacs-tools.el

tools/mcp-server-emacs-tools-eval-elisp.el

Rename shadowed lambda parameter from `t` (which shadows the built-in constant) to `tool` for better code clarity and to avoid conflicts. Also fix required field format in eval-elisp schema from vector to list.

Document known security model limitations including blocklist bypass via indirection, macro evaluation risks, and dynamic function construction. Add recommendations for high-security requirements. Also convert internal security variables to defcustom for better user configurability and fix variable references throughout the codebase.

- Fix typos: "parantheses" → "parentheses", "ELisp" → "Elisp" - Add comprehensive Elisp coding conventions section covering naming, variables, comparisons, error handling, JSON schemas, and more - Document self-registration pattern for tools with step-by-step guide - Add tool visibility and filtering behavior explanation - Update minimum Emacs version badge to 27.1+ - Clarify throw/catch usage in message handler with inline comment - Bump version to 0.4.0

…edly Set mcp-server-running to nil when the Unix socket server process terminates unexpectedly to ensure the main server is aware of the transport failure.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Check if transport process is actually alive before blocking restart. When server flag is set but transport is dead, automatically clean up the stale state and allow restart to proceed.

rhblind linked an issue Jan 8, 2026 that may be closed by this pull request

Feature: Add error diagnostics tool #5

Closed

rhblind marked this pull request as draft January 8, 2026 01:10

rhblind requested a review from Copilot January 8, 2026 01:10

Copilot started reviewing on behalf of rhblind January 8, 2026 01:11 View session

Copilot AI reviewed Jan 8, 2026

View reviewed changes

fix: Handle missing file variables in load-path setup

7df3e33

Add nil checks for load-file-name and buffer-file-name before using them to construct the tools directory path. This prevents errors when these variables are not set in certain loading contexts.

rhblind requested a review from Copilot January 8, 2026 07:29

Copilot started reviewing on behalf of rhblind January 8, 2026 07:29 View session

Copilot AI reviewed Jan 8, 2026

View reviewed changes

mcp-server-emacs-tools.el Outdated Show resolved Hide resolved

tools/mcp-server-emacs-tools-eval-elisp.el Show resolved Hide resolved

docs(README): Add warning about eval-elisp security risks

5059813

Emphasize that eval-elisp enables arbitrary code execution and poses inherent security risks when granting LLMs access to evaluate Elisp.

rhblind requested a review from Copilot January 8, 2026 07:52

Copilot started reviewing on behalf of rhblind January 8, 2026 07:52 View session

Copilot AI reviewed Jan 8, 2026

View reviewed changes

README.md Outdated Show resolved Hide resolved

rhblind requested a review from Copilot January 8, 2026 08:02

Copilot started reviewing on behalf of rhblind January 8, 2026 08:03 View session

Copilot AI reviewed Jan 8, 2026

View reviewed changes

README.md Show resolved Hide resolved

CLAUDE.md Show resolved Hide resolved

rhblind requested a review from Copilot January 8, 2026 08:39

Copilot started reviewing on behalf of rhblind January 8, 2026 08:40 View session

Copilot AI reviewed Jan 8, 2026

View reviewed changes

test/unit/test-mcp-emacs-tools.el Outdated Show resolved Hide resolved

test/unit/test-mcp-emacs-tools.el Outdated Show resolved Hide resolved

tools/mcp-server-emacs-tools-eval-elisp.el Outdated Show resolved Hide resolved

rhblind added 3 commits January 8, 2026 09:48

fix(tests): Rename lambda parameter from t to tool

b885a2d

Rename shadowed lambda parameter from `t` (which shadows the built-in constant) to `tool` for better code clarity and to avoid conflicts. Also fix required field format in eval-elisp schema from vector to list.

rhblind requested a review from Copilot January 8, 2026 09:28

Copilot started reviewing on behalf of rhblind January 8, 2026 09:28 View session

fix(transport-unix): notify server when transport terminates unexpect…

18e1e19

…edly Set mcp-server-running to nil when the Unix socket server process terminates unexpectedly to ensure the main server is aware of the transport failure.

Copilot AI reviewed Jan 8, 2026

View reviewed changes

docs: Add CHANGELOG documenting releases 0.1.0 through 0.4.0

3e475a0

feat(server): add recovery from stale server state

ee1d845

Check if transport process is actually alive before blocking restart. When server flag is set but transport is dead, automatically clean up the stale state and allow restart to proceed.

rhblind marked this pull request as ready for review January 8, 2026 09:57

rhblind merged commit bda5401 into main Jan 8, 2026
8 checks passed

Conversation

rhblind commented Jan 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant