Skip to content

feat: Add mcp tools support#282

Merged
yinwm merged 49 commits intosipeed:mainfrom
yuchou87:mcp-tools-support
Mar 2, 2026
Merged

feat: Add mcp tools support#282
yinwm merged 49 commits intosipeed:mainfrom
yuchou87:mcp-tools-support

Conversation

@yuchou87
Copy link
Contributor

Add Model Context Protocol (MCP) Integration with Docker Support

Overview

This PR introduces comprehensive Model Context Protocol (MCP) integration to picoclaw, enabling dynamic tool registration from external MCP servers. Additionally, it provides production-ready Docker deployment options with full MCP runtime support.

Key Features

MCP Integration

  • MCP SDK Integration: Implemented using github.com/modelcontextprotocol/go-sdk v1.3.0
  • Multiple Transport Protocols: Support for stdio (subprocess), SSE, and HTTP transports
  • Automatic Tool Registration: MCP tools are automatically registered to the agent with mcp_{server}_{tool} naming convention
  • Environment Management: Support for both inline environment variables and .env file loading with proper priority handling
  • HTTP Headers Support: Custom header injection for HTTP/SSE transport modes
  • Resource Cleanup: Proper lifecycle management with automatic cleanup on agent shutdown

Docker Support

  • Dual Image Strategy:
    • Minimal Image (Alpine-based): Lightweight runtime (~30MB) for basic deployments
    • Full Image (Node.js 24): Complete MCP tooling support including npx, npm, git, python3, and uv
  • Separate Compose Files: Independent docker-compose.yml (minimal) and docker-compose.full.yml (full-featured)
  • Makefile Integration: Convenient targets for building, running, and testing Docker containers
  • MCP Tool Validation: Automated test script to verify all MCP tools are properly installed

Configuration Enhancements

  • Added tools.mcp section to configuration schema
  • Provided 5 example MCP server configurations in config.example.json
  • Support for envFile paths with environment variable merging (envFile < config.Env priority)
  • Global MCP enable/disable switch with tools.mcp.enabled

Technical Changes

Core Implementation

  • pkg/mcp/manager.go (432 lines): MCP server connection manager with transport auto-detection
  • pkg/tools/mcp_tool.go (119 lines): MCP tool wrapper for agent integration
  • pkg/agent/loop.go: Integrated MCP manager with tool registration loop and cleanup
  • pkg/config/config.go: Extended configuration schema with MCPServerConfig struct

Docker Infrastructure

  • Dockerfile.full (46 lines): Node.js 24-based image with git, python3, and uv
  • docker-compose.full.yml (44 lines): Full-featured compose configuration with npm-cache volume
  • scripts/test-docker-mcp.sh (49 lines): Validation script for MCP tools
  • Makefile: Added docker-* targets for build, run, and test operations

Testing

  • pkg/mcp/manager_test.go (182 lines): 9 comprehensive tests for envFile parsing and configuration
  • pkg/tools/mcp_tool_test.go (456 lines): Integration tests for MCP tool execution

Statistics

  • 13 files changed: 1,543 insertions(+), 5 deletions(-)
  • Major additions:
    • MCP implementation: 1,189 lines (manager + tools + tests)
    • Docker support: 139 lines (Dockerfile.full + compose + scripts)
    • Configuration examples: 57 lines

Bug Fixes

  • Fixed uv installation path with proper symlinks to /usr/local/bin
  • Resolved Docker Compose v2 migration (all commands use docker compose instead of docker-compose)
  • Fixed "No services to build" error by using explicit service names
  • Corrected test script permissions and entrypoint override for non-interactive testing
  • Fixed go.mod formatting

Testing

  • ✅ All existing tests passing
  • ✅ 9 new MCP manager tests
  • ✅ 456 lines of MCP tool integration tests
  • ✅ Docker builds successful (both minimal and full images)
  • ✅ MCP tools verified: npx v11.8.0, npm v11.8.0, node v24.13.1, git v2.39.5, python 3.11.2, uv

Usage Examples

Build and Run with Docker

# Build full-featured Docker image
make docker-build-full

# Run interactive agent with MCP support
make docker-run-agent-full

# Test MCP tools installation
make docker-test

Configuration Example

Add MCP servers to your config.json:

{
  "tools": {
    "mcp": {
      "enabled": true,
      "servers": {
        "filesystem": {
          "enabled": true,
          "command": "npx",
          "args": ["-y", "@modelcontextprotocol/server-filesystem", "/workspace"],
          "env": {}
        },
        "brave-search": {
          "enabled": true,
          "command": "npx",
          "args": ["-y", "@modelcontextprotocol/server-brave-search"],
          "env": {
            "BRAVE_API_KEY": "YOUR_BRAVE_API_KEY"
          },
          "envFile": ".env"
        }
      }
    }
  }
}

Breaking Changes

None. This is a purely additive feature that maintains full backward compatibility.

Implement comprehensive MCP support with stdio/HTTP/SSE transports, environment variable configuration (env and envFile), custom headers, tool registration, and automatic resource cleanup. Includes full test coverage and VSCode-compatible configuration.

- Added pkg/mcp/manager.go for server lifecycle management
- Added pkg/tools/mcp_tool.go for tool wrapping
- Integrated into agent loop with cleanup
- Support for envFile loading (.env format)
- Headers injection for HTTP/SSE authentication
- Example configs for filesystem, github, brave-search, postgres
Add Dockerfile.full with Debian-based runtime including git, nodejs, npm, python3, and uv for MCP servers. Add docker-compose.full.yml with npm cache optimization. Add Makefile targets for docker-build-full, docker-run-full, and docker-test. Add test script for MCP tools validation.
Replace debian:bookworm-slim with node:24-bookworm-slim to:
- Use latest Node.js 24 LTS and npm
- Fix npm version compatibility issues (npm@11 requires node >=20.17)
- Simplify Dockerfile by removing nodejs/npm installation
- Better optimization from official Node.js image

Changes:
- Dockerfile.full: use node:24-bookworm-slim base
- Remove nodejs, npm installation steps
- Remove npm upgrade step (included in base image)
- Update Makefile descriptions to reflect Node.js 24
…mode

Add --entrypoint sh flag to docker-compose run commands in test script
to bypass picoclaw agent's interactive mode. This allows direct command
execution for testing MCP tools.

Changes:
- Add --entrypoint sh to all docker-compose run commands
- Use SERVICE variable for better maintainability
- Simplify command syntax: sh -c 'cmd' → -c 'cmd'
Replace docker-compose (v1) with docker compose (v2) command syntax
across all files. Docker Compose v2 is now the default in modern
Docker installations and uses 'docker compose' instead of 'docker-compose'.

Changes:
- scripts/test-docker-mcp.sh: update all 8 docker-compose commands
- Makefile: update all 8 docker-compose commands in docker-* targets
- No changes to file names (docker-compose.full.yml remains as-is)

Compatibility: Requires Docker with Compose v2 plugin (Docker Desktop
or docker-compose-plugin package)
Symlink uv from /root/.cargo/bin to /usr/local/bin to make it
accessible without relying on ENV PATH setting. Add version check
to verify successful installation during build.

Changes:
- Symlink uv to /usr/local/bin/uv
- Add 'uv --version' validation step
- Remove ENV PATH setting (no longer needed)

Fixes: uv: not found error in test script
Fix uv symlink path from /root/.cargo/bin to /root/.local/bin.
The uv installer puts binaries in ~/.local/bin, not ~/.cargo/bin.

Changes:
- Update uv symlink source: /root/.local/bin/uv
- Add uvx symlink as well (installed alongside uv)

Fixes: /bin/sh: 1: uv: not found error during build
Add --profile gateway --profile agent flags to docker build commands
to ensure services are built even when using profiles in compose files.

Without profiles specified, docker compose build skips all services
that have a profile defined, resulting in 'No services to build' warning.

Changes:
- docker-build: add --profile flags
- docker-build-full: add --profile flags

Fixes: WARN[0000] No services to build
Replace --profile flags with explicit service names in build commands.
The 'docker compose build' command does not support --profile flag;
profiles are only used for runtime operations like 'up' and 'run'.

Changes:
- docker-build: specify picoclaw-agent picoclaw-gateway
- docker-build-full: specify picoclaw-agent picoclaw-gateway

Fixes: unknown flag: --profile error
Add blank line for better formatting consistency
Make scripts/test-docker-mcp.sh executable
Copilot AI review requested due to automatic review settings February 16, 2026 09:09
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds Model Context Protocol (MCP) support to picoclaw by introducing an MCP connection manager, wrapping MCP tools into the existing tool system, and providing “full” Docker tooling/runtime artifacts to run MCP servers (e.g., via npx) in containers.

Changes:

  • Added MCP server manager (pkg/mcp) with stdio and SSE/HTTP transports, env/envFile support, and tool discovery.
  • Added MCP tool wrapper (pkg/tools) and integrated MCP tool registration + cleanup into the agent loop.
  • Added Docker “full” runtime support (Dockerfile + compose + test script) and updated config examples / Makefile targets.

Reviewed changes

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

Show a summary per file
File Description
scripts/test-docker-mcp.sh Adds a Docker Compose-based validation script for MCP runtime tools (npx/npm/node/git/python/uv).
pkg/tools/mcp_tool.go Introduces MCPTool wrapper implementing the project Tool interface.
pkg/tools/mcp_tool_test.go Adds unit/integration-style tests for MCPTool behavior and content extraction.
pkg/mcp/manager.go Adds MCP Manager to connect to servers, list tools, call tools, and close connections.
pkg/mcp/manager_test.go Adds tests for .env parsing and envFile/config merge priority.
pkg/agent/loop.go Initializes MCP manager, registers MCP tools, and attempts cleanup on Stop().
pkg/config/config.go Extends config schema with tools.mcp and server definitions.
config/config.example.json Adds MCP configuration examples (including envFile and headers).
Dockerfile.full Adds a Node-based runtime image with additional tooling for MCP servers.
docker-compose.full.yml Adds a “full” compose setup for agent and gateway using Dockerfile.full.
Makefile Adds docker build/run/test/clean targets (minimal + full).
go.mod Adds MCP SDK dependency and an extra indirect require line.
go.sum Adds checksums for newly introduced dependencies.

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

- Add errors.Join to return aggregated error when all enabled MCP servers fail
- Track enabled server count separately from total configured servers
- Return error only when all servers fail, not for partial failures
- Improve logging with accurate server counts (enabled vs connected)
- Maintains fault tolerance: partial failures don't stop initialization
- Resolve relative envFile paths relative to workspace instead of CWD
- Add filepath import for path operations
- Pass workspace path to goroutines for path resolution
- Improves portability in Docker environments where CWD may vary
- Absolute envFile paths continue to work as before
- Move standalone indirect require line into existing require block
- Maintain alphabetical ordering of dependencies
- Keep module file stable and avoid churn
- Change NewMCPTool to accept MCPManager interface instead of concrete *mcp.Manager
- Remove unused mcpPkg import from mcp_tool.go
- Remove newMCPToolForTest helper function as NewMCPTool now accepts interface
- Update all tests to use NewMCPTool directly with MockMCPManager
- Improves testability and follows dependency inversion principle
- Handle json.RawMessage and []byte types by direct unmarshal
- Use JSON marshal/unmarshal for struct types to preserve schema
- Add test case for json.RawMessage schema
- Fixes issue where non-map schemas returned empty object

This fixes GitHub Copilot feedback that Parameters() was dropping
tool schema when InputSchema wasn't already map[string]interface{}
- Defer MCP server initialization to Run() using agent's context
- Add mcpConfig and mcpInitOnce fields to AgentLoop
- Use sync.Once to ensure MCP loads exactly once with proper context
- Prevents orphaned subprocesses and resource leaks on cancellation

This fixes GitHub Copilot feedback that MCP connections with
context.Background() won't terminate when the agent stops, causing
potential resource leaks and orphaned stdio/SSE connections.
- Add defer in Run() to guarantee MCP connection cleanup
- Handles both normal termination and context cancellation
- Prevents resource leaks when Run() exits via ctx.Done()
- MCP Manager.Close() is idempotent, safe to call from both defer and Stop()

This fixes GitHub Copilot feedback that MCP cleanup only happened in
Stop() but Run() could return on ctx.Done() without cleanup, causing
subprocess/session leaks on normal cancellation shutdown.
Critical bug fix:
- MCP tools were never registered because servers loaded in Run()
  but tool registration happened in NewAgentLoop() with empty manager
- Move MCP tool registration from createToolRegistry to Run()
- Register MCP tools for both main agent and subag after successful server loading
- Add subagentManager field to AgentLoop for dynamic registration
- Add tool_count logging for better observability

This ensures MCP tools are properly available to both agent and subagents.
@Danieldd28
Copy link
Collaborator

I reviewed this PR from the perspective of PicoClaw's resource optimization goals (RAM usage > binary size). The MCP integration itself is a valuable feature, and the overall architecture is solid lazy initialization via sync.Once, parallel server startup with sync.WaitGroup, and proper cleanup in both defer and
Stop() are all well done. That said, there are several resource-related concerns worth addressing before merge.

  1. Duplicate MCP Tool Instances (RAM)

Each MCP tool discovered from a server is instantiated twice -- once for the main agent and once for the subagent:

mcpTool := tools.NewMCPTool(al.mcpManager, serverName, tool)
al.tools.Register(mcpTool)
if al.subagentManager != nil {
    al.subagentManager.RegisterTool(tools.NewMCPTool(al.mcpManager, serverName, tool))
}

While the underlying MCPManager is shared (good), each MCPTool wrapper struct is allocated separately. For a setup with, say, 4 MCP servers exposing 10 tools each, that is 80 struct allocations instead of 40. Each wrapper carries a pointer to the manager, the server name string, and the full *mcp.Tool reference (which includes the input schema).

This compounds an existing architectural issue in PicoClaw where
createToolRegistry()
is already called twice (once for the agent, once for subagent), duplicating all built-in tools as well. This PR extends that pattern to MCP tools.

Recommendation: consider sharing a single read-only tool registry between the agent and subagent, or at minimum use a ToolRegistryView wrapper that filters out spawn/subagent tools instead of cloning the entire registry. If that is out of scope for this PR, it should at least be documented as tracked tech debt.

  1. Full Config Reference Retained in AgentLoop (RAM)

The PR adds a new field to
AgentLoop
:

mcpConfig *config.Config

This stores a reference to the entire application config struct for lazy initialization in
Run(). While this is a pointer (so the direct cost is small), it prevents the config object from being garbage collected for the entire lifetime of the agent loop, and it couples the agent more tightly to the full config surface.

Since the only usage is al.mcpManager.LoadFromConfig(ctx, al.mcpConfig), and LoadFromConfig only reads cfg.Tools.MCP and cfg.WorkspacePath(), consider storing just the MCP-specific config:

mcpConfig     config.MCPConfig
workspacePath string

This makes the dependency explicit and allows the rest of the config to be GC'd if nothing else holds it.

  1. Default Config Pre-populates Disabled Servers (RAM)

DefaultConfig() now ships with four pre-populated MCP server entries (filesystem, github, brave-search, postgres), all disabled:

MCP: MCPConfig{
    Enabled: false,
    Servers: map[string]MCPServerConfig{
        "filesystem":   { Enabled: false, Command: "npx", Args: [...] },
        "github":       { Enabled: false, Command: "npx", Args: [...], Env: {...} },
        "brave-search": { Enabled: false, Command: "npx", Args: [...], Env: {...} },
        "postgres":     { Enabled: false, Command: "npx", Args: [...] },
    },
},

Every PicoClaw instance allocates this map, its string keys, the MCPServerConfig structs, and their nested slices/maps, even if MCP is never used. The cost per instance is small (a few hundred bytes), but it is unnecessary since these examples already exist in
config/config.example.json
.

  1. Dockerfile.full Image Size (Binary/Deployment)

The new Dockerfile.full switches from alpine:3.23 (the base for the existing Dockerfile) to node:24-bookworm-slim and installs Python 3, pip, venv, git, curl, and uv. This is understandable since many MCP servers are Node.js or Python processes, but it transforms PicoClaw's Docker footprint from roughly 15-20MB (Alpine + static Go binary) to an estimated 800MB+.

  1. MCP Tool Schema Sent to LLM on Every Call (Token Cost)

When MCP is enabled and servers are connected, every MCP tool's schema is serialized and sent to the LLM on every chat request. For a setup with many MCP servers, this could add significant token overhead. This is inherent to how tool-use works with LLMs, but it is worth considering:

A configurable limit on the maximum number of MCP tools exposed to the LLM at once.
Or a mechanism to selectively enable/disable individual tools per server (beyond the current per-server toggle), so users can prune tools they do not need.
This is not a blocker, but something to keep in mind as MCP adoption grows.

@Leeaandrob
Copy link
Collaborator

@Zepan This PR directly addresses roadmap issue #290 (MCP Support — priority: high). MCP is one of the most requested features and is explicitly on the project board.

+1693 lines is substantial but MCP requires protocol implementation, transport layer, and tool registration. Worth a careful review given the scope.

Recommendation: Review and merge. This is a high-priority roadmap item. Consider whether this implementation aligns with the architecture vision before merging.

Replace full *config.Config reference with config.MCPConfig value type
in AgentLoop to allow garbage collection of unused configuration data.

Changes:
- AgentLoop now stores only MCPConfig and workspacePath (minimal deps)
- Add mcp.Manager.LoadFromMCPConfig() for minimal dependency version
- Keep LoadFromConfig() for backward compatibility
- Full Config object can be GC'd after NewAgentLoop() returns

This optimization reduces memory usage by not holding references to
unused channel, provider, gateway, and device configurations.
Remove pre-populated example servers (filesystem, github, brave-search,
postgres) from DefaultConfig() to reduce memory footprint per instance.

Changes:
- Set MCP.Servers to empty map instead of 4 example servers
- Reduces default config size by ~500 bytes per instance
- Users should add MCP servers via config.json or documentation

Example configurations are still available in:
- README.md MCP section
- config.example.json
- Official MCP documentation

This optimization benefits deployments with many agent instances.
Replace node:24-bookworm-slim with node:24-alpine3.23 to reduce
image size and improve build efficiency.

Changes:
- Base image: node:24-bookworm-slim → node:24-alpine3.23
- Package manager: apt-get → apk
- Package names: python3-pip → py3-pip
- Remove python3-venv (included in Alpine Python3)
- Use apk --no-cache for cleaner image layers

Expected benefits:
- Reduce base image size by ~100-200MB (30-40% reduction)
- Faster image pulls and container startup
- Full MCP support maintained (Node.js, Python, uv)

Estimated final image size: ~600-700MB (vs ~800MB before)
@yuchou87
Copy link
Contributor Author

I reviewed this PR from the perspective of PicoClaw's resource optimization goals (RAM usage > binary size). The MCP integration itself is a valuable feature, and the overall architecture is solid lazy initialization via sync.Once, parallel server startup with sync.WaitGroup, and proper cleanup in both defer and Stop() are all well done. That said, there are several resource-related concerns worth addressing before merge.

  1. Duplicate MCP Tool Instances (RAM)

Each MCP tool discovered from a server is instantiated twice -- once for the main agent and once for the subagent:

mcpTool := tools.NewMCPTool(al.mcpManager, serverName, tool)
al.tools.Register(mcpTool)
if al.subagentManager != nil {
    al.subagentManager.RegisterTool(tools.NewMCPTool(al.mcpManager, serverName, tool))
}

While the underlying MCPManager is shared (good), each MCPTool wrapper struct is allocated separately. For a setup with, say, 4 MCP servers exposing 10 tools each, that is 80 struct allocations instead of 40. Each wrapper carries a pointer to the manager, the server name string, and the full *mcp.Tool reference (which includes the input schema).

This compounds an existing architectural issue in PicoClaw where createToolRegistry() is already called twice (once for the agent, once for subagent), duplicating all built-in tools as well. This PR extends that pattern to MCP tools.

Recommendation: consider sharing a single read-only tool registry between the agent and subagent, or at minimum use a ToolRegistryView wrapper that filters out spawn/subagent tools instead of cloning the entire registry. If that is out of scope for this PR, it should at least be documented as tracked tech debt.

  1. Full Config Reference Retained in AgentLoop (RAM)

The PR adds a new field to AgentLoop :

mcpConfig *config.Config

This stores a reference to the entire application config struct for lazy initialization in Run(). While this is a pointer (so the direct cost is small), it prevents the config object from being garbage collected for the entire lifetime of the agent loop, and it couples the agent more tightly to the full config surface.

Since the only usage is al.mcpManager.LoadFromConfig(ctx, al.mcpConfig), and LoadFromConfig only reads cfg.Tools.MCP and cfg.WorkspacePath(), consider storing just the MCP-specific config:

mcpConfig     config.MCPConfig
workspacePath string

This makes the dependency explicit and allows the rest of the config to be GC'd if nothing else holds it.

  1. Default Config Pre-populates Disabled Servers (RAM)

DefaultConfig() now ships with four pre-populated MCP server entries (filesystem, github, brave-search, postgres), all disabled:

MCP: MCPConfig{
    Enabled: false,
    Servers: map[string]MCPServerConfig{
        "filesystem":   { Enabled: false, Command: "npx", Args: [...] },
        "github":       { Enabled: false, Command: "npx", Args: [...], Env: {...} },
        "brave-search": { Enabled: false, Command: "npx", Args: [...], Env: {...} },
        "postgres":     { Enabled: false, Command: "npx", Args: [...] },
    },
},

Every PicoClaw instance allocates this map, its string keys, the MCPServerConfig structs, and their nested slices/maps, even if MCP is never used. The cost per instance is small (a few hundred bytes), but it is unnecessary since these examples already exist in config/config.example.json .

  1. Dockerfile.full Image Size (Binary/Deployment)

The new Dockerfile.full switches from alpine:3.23 (the base for the existing Dockerfile) to node:24-bookworm-slim and installs Python 3, pip, venv, git, curl, and uv. This is understandable since many MCP servers are Node.js or Python processes, but it transforms PicoClaw's Docker footprint from roughly 15-20MB (Alpine + static Go binary) to an estimated 800MB+.

  1. MCP Tool Schema Sent to LLM on Every Call (Token Cost)

When MCP is enabled and servers are connected, every MCP tool's schema is serialized and sent to the LLM on every chat request. For a setup with many MCP servers, this could add significant token overhead. This is inherent to how tool-use works with LLMs, but it is worth considering:

A configurable limit on the maximum number of MCP tools exposed to the LLM at once. Or a mechanism to selectively enable/disable individual tools per server (beyond the current per-server toggle), so users can prune tools they do not need. This is not a blocker, but something to keep in mind as MCP adoption grows.

I reviewed this PR from the perspective of PicoClaw's resource optimization goals (RAM usage > binary size). The MCP integration itself is a valuable feature, and the overall architecture is solid lazy initialization via sync.Once, parallel server startup with sync.WaitGroup, and proper cleanup in both defer and Stop() are all well done. That said, there are several resource-related concerns worth addressing before merge.

  1. Duplicate MCP Tool Instances (RAM)

Each MCP tool discovered from a server is instantiated twice -- once for the main agent and once for the subagent:

mcpTool := tools.NewMCPTool(al.mcpManager, serverName, tool)
al.tools.Register(mcpTool)
if al.subagentManager != nil {
    al.subagentManager.RegisterTool(tools.NewMCPTool(al.mcpManager, serverName, tool))
}

While the underlying MCPManager is shared (good), each MCPTool wrapper struct is allocated separately. For a setup with, say, 4 MCP servers exposing 10 tools each, that is 80 struct allocations instead of 40. Each wrapper carries a pointer to the manager, the server name string, and the full *mcp.Tool reference (which includes the input schema).

This compounds an existing architectural issue in PicoClaw where createToolRegistry() is already called twice (once for the agent, once for subagent), duplicating all built-in tools as well. This PR extends that pattern to MCP tools.

Recommendation: consider sharing a single read-only tool registry between the agent and subagent, or at minimum use a ToolRegistryView wrapper that filters out spawn/subagent tools instead of cloning the entire registry. If that is out of scope for this PR, it should at least be documented as tracked tech debt.

  1. Full Config Reference Retained in AgentLoop (RAM)

The PR adds a new field to AgentLoop :

mcpConfig *config.Config

This stores a reference to the entire application config struct for lazy initialization in Run(). While this is a pointer (so the direct cost is small), it prevents the config object from being garbage collected for the entire lifetime of the agent loop, and it couples the agent more tightly to the full config surface.

Since the only usage is al.mcpManager.LoadFromConfig(ctx, al.mcpConfig), and LoadFromConfig only reads cfg.Tools.MCP and cfg.WorkspacePath(), consider storing just the MCP-specific config:

mcpConfig     config.MCPConfig
workspacePath string

This makes the dependency explicit and allows the rest of the config to be GC'd if nothing else holds it.

  1. Default Config Pre-populates Disabled Servers (RAM)

DefaultConfig() now ships with four pre-populated MCP server entries (filesystem, github, brave-search, postgres), all disabled:

MCP: MCPConfig{
    Enabled: false,
    Servers: map[string]MCPServerConfig{
        "filesystem":   { Enabled: false, Command: "npx", Args: [...] },
        "github":       { Enabled: false, Command: "npx", Args: [...], Env: {...} },
        "brave-search": { Enabled: false, Command: "npx", Args: [...], Env: {...} },
        "postgres":     { Enabled: false, Command: "npx", Args: [...] },
    },
},

Every PicoClaw instance allocates this map, its string keys, the MCPServerConfig structs, and their nested slices/maps, even if MCP is never used. The cost per instance is small (a few hundred bytes), but it is unnecessary since these examples already exist in config/config.example.json .

  1. Dockerfile.full Image Size (Binary/Deployment)

The new Dockerfile.full switches from alpine:3.23 (the base for the existing Dockerfile) to node:24-bookworm-slim and installs Python 3, pip, venv, git, curl, and uv. This is understandable since many MCP servers are Node.js or Python processes, but it transforms PicoClaw's Docker footprint from roughly 15-20MB (Alpine + static Go binary) to an estimated 800MB+.

  1. MCP Tool Schema Sent to LLM on Every Call (Token Cost)

When MCP is enabled and servers are connected, every MCP tool's schema is serialized and sent to the LLM on every chat request. For a setup with many MCP servers, this could add significant token overhead. This is inherent to how tool-use works with LLMs, but it is worth considering:

A configurable limit on the maximum number of MCP tools exposed to the LLM at once. Or a mechanism to selectively enable/disable individual tools per server (beyond the current per-server toggle), so users can prune tools they do not need. This is not a blocker, but something to keep in mind as MCP adoption grows.

@Danieldd28
Thank you for your thorough review and valuable feedback on this MR!

I've addressed the following optimization issues:

✅ Fixed in this PR:

1. Full Config Reference Retained in AgentLoop (RAM)

Resolved by storing only MCPConfig and workspacePath instead of the full *config.Config reference, allowing unused configuration to be garbage collected.

  • Commit: 6892d00 - perf(agent): reduce memory footprint by storing minimal MCP dependencies
  • Changes: AgentLoop now holds only the minimal dependencies needed for MCP initialization
  • Impact: Full Config object can be GC'd after NewAgentLoop() returns

2. Default Config Pre-populates Disabled Servers (RAM)

Resolved by removing the 4 example MCP servers from DefaultConfig(), reducing memory footprint by ~500 bytes per instance.

  • Commit: 4113190 - chore(config): remove example MCP servers from default config
  • Changes: Set MCP.Servers to empty map instead of 4 pre-populated examples
  • Impact: Reduces default config size, especially beneficial for multi-instance deployments
  • Note: Example configurations remain available in config.example.json

3. Dockerfile.full Image Size (Binary/Deployment)

Resolved by migrating from node:24-bookworm-slim to node:24-alpine3.23, reducing the final image size from 464MB to 316MB (32% reduction).

  • Commit: e38364b - build(docker): migrate full image from Debian to Alpine base
  • Changes:
    • Base image: node:24-bookworm-slimnode:24-alpine3.23
    • Package manager: apt-getapk
    • Optimized dependency installation
  • Impact: 148MB reduction in final image size, faster pulls and container startup

📋 Deferred for future optimization:

1. Duplicate MCP Tool Instances (RAM)

This requires significant refactoring (view pattern or lazy copy approach). I recommend addressing this in a separate PR to keep changes focused and reviewable.

Reasoning:

  • Requires substantial code changes (~150+ lines for view pattern, or ~30 lines for lazy copy)
  • Involves architectural decisions (registry view vs. lazy duplication)
  • Best handled as a dedicated optimization PR with thorough testing

2. MCP Tool Schema Sent to LLM on Every Call (Token Cost)

This optimization requires additional API design (per-tool enable/disable, max tool limits). I suggest implementing this in a follow-up PR after gathering more user feedback on the feature.

Reasoning:

  • Requires new configuration options and tool filtering logic
  • Token cost optimization needs real-world usage data to determine optimal approach
  • Better addressed after users have experience with the MCP feature

@yinwm
Copy link
Collaborator

yinwm commented Feb 28, 2026

@yuchou87 please resolve conflicts

# Conflicts:
#	go.mod
#	go.sum
#	pkg/agent/loop.go
#	pkg/config/config.go
@yinwm
Copy link
Collaborator

yinwm commented Feb 28, 2026

@yuchou87 Linter failed, please fix

- Fix gci import ordering in manager.go, manager_test.go
- Fix gofmt formatting in loop.go, manager.go, mcp_tool.go, mcp_tool_test.go
- Fix gofumpt formatting in manager_test.go
- Fix golines line length issues in manager.go, mcp_tool_test.go
- Fix wastedassign: replace redundant zero-value init with var declaration in loop.go
Copilot AI review requested due to automatic review settings March 1, 2026 00:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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


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

yuchou87 added 2 commits March 1, 2026 10:56
- Avoid logging sensitive cfg.Args in ConnectServer; log args_count instead
- Sanitize server/tool name components in MCPTool.Name() to ensure valid
  identifiers for downstream providers (lowercase, [a-z0-9_-] only)
- Add slack as 5th MCP server example in config.example.json
- Move Dockerfile.full and docker-compose.full.yml into docker/ directory
  for consistency with existing docker/Dockerfile and docker/docker-compose.yml
- Fix all Makefile docker-* targets to reference correct compose file paths
- Fix docker/docker-compose.full.yml build context (.. ) and volume paths
- Fix scripts/test-docker-mcp.sh compose file path and replace cowsay test
  with actual @modelcontextprotocol/server-filesystem MCP server test
server-filesystem does not support --help; use timeout + /dev/null stdin
to verify installation and startup without hanging the test script
Copilot AI review requested due to automatic review settings March 1, 2026 03:24
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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


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

…rning

- MCPTool.Name(): append FNV-32a hash of original (unsanitized) server+tool
  names whenever sanitization is lossy or total length exceeds 64 chars,
  ensuring names that differ only in disallowed characters remain distinct
- ToolRegistry.Register(): emit warn log when a tool registration overwrites
  an existing tool with the same name, making collisions observable
- scripts/test-docker-mcp.sh: switch shebang from #/bin/bash /Users/yuchou/Work/klook-calendar/klook-google-cal-sync/src/googlecalconversrv/bin/start.sh to #  for portability on minimal distros and Nix environments
@yuchou87
Copy link
Contributor Author

yuchou87 commented Mar 1, 2026

@yuchou87 Linter failed, please fix

done

@yinwm
Copy link
Collaborator

yinwm commented Mar 2, 2026

Code review

Found 2 issues:

  1. TOCTOU race condition in CallTool/Close - m.closed check and wg.Add(1) are not atomic. After Close() sets m.closed=true and calls wg.Wait(), CallTool() may still call wg.Add(1), causing sync: WaitGroup misuse: Add called concurrently with Wait panic.

picoclaw/pkg/mcp/manager.go

Lines 168 to 199 in a2591e0

if serverCfg.EnvFile != "" && !filepath.IsAbs(serverCfg.EnvFile) {
if workspace == "" {
err := fmt.Errorf(
"workspace path is empty while resolving relative envFile %q for server %s",
serverCfg.EnvFile,
name,
)
logger.ErrorCF("mcp", "Invalid MCP server configuration",
map[string]any{
"server": name,
"env_file": serverCfg.EnvFile,
"error": err.Error(),
})
errs <- err
return
}
serverCfg.EnvFile = filepath.Join(workspace, serverCfg.EnvFile)
}
if err := m.ConnectServer(ctx, name, serverCfg); err != nil {
logger.ErrorCF("mcp", "Failed to connect to MCP server",
map[string]any{
"server": name,
"error": err.Error(),
})
errs <- fmt.Errorf("failed to connect to server %s: %w", name, err)
}
}(name, serverCfg, workspacePath)
}
wg.Wait()
close(errs)

  1. Resource leak on partial MCP init failure - defer mcpManager.Close() is only registered when LoadFromMCPConfig succeeds. However, LoadFromMCPConfig may partially connect some servers before failing (see lines 1022-1055 where goroutines connect servers independently). These partial connections are not cleaned up, causing resource leaks (child processes, network connections).

picoclaw/pkg/agent/loop.go

Lines 50 to 115 in a2591e0

Channel string // Target channel for tool execution
ChatID string // Target chat ID for tool execution
UserMessage string // User message content (may include prefix)
DefaultResponse string // Response when LLM returns empty
EnableSummary bool // Whether to trigger summarization
SendResponse bool // Whether to send response via bus
NoHistory bool // If true, don't load session history (for heartbeat)
}
const defaultResponse = "I've completed processing but have no response to give. Increase `max_tool_iterations` in config.json."
func NewAgentLoop(
cfg *config.Config,
msgBus *bus.MessageBus,
provider providers.LLMProvider,
) *AgentLoop {
registry := NewAgentRegistry(cfg, provider)
// Register shared tools to all agents
registerSharedTools(cfg, msgBus, registry, provider)
// Set up shared fallback chain
cooldown := providers.NewCooldownTracker()
fallbackChain := providers.NewFallbackChain(cooldown)
// Create state manager using default agent's workspace for channel recording
defaultAgent := registry.GetDefaultAgent()
var stateManager *state.Manager
if defaultAgent != nil {
stateManager = state.NewManager(defaultAgent.Workspace)
}
return &AgentLoop{
bus: msgBus,
cfg: cfg,
registry: registry,
state: stateManager,
summarizing: sync.Map{},
fallback: fallbackChain,
}
}
// registerSharedTools registers tools that are shared across all agents (web, message, spawn).
func registerSharedTools(
cfg *config.Config,
msgBus *bus.MessageBus,
registry *AgentRegistry,
provider providers.LLMProvider,
) {
for _, agentID := range registry.ListAgentIDs() {
agent, ok := registry.GetAgent(agentID)
if !ok {
continue
}
// Web tools
if searchTool := tools.NewWebSearchTool(tools.WebSearchToolOptions{
BraveAPIKey: cfg.Tools.Web.Brave.APIKey,
BraveMaxResults: cfg.Tools.Web.Brave.MaxResults,
BraveEnabled: cfg.Tools.Web.Brave.Enabled,
TavilyAPIKey: cfg.Tools.Web.Tavily.APIKey,
TavilyBaseURL: cfg.Tools.Web.Tavily.BaseURL,
TavilyMaxResults: cfg.Tools.Web.Tavily.MaxResults,
TavilyEnabled: cfg.Tools.Web.Tavily.Enabled,
DuckDuckGoMaxResults: cfg.Tools.Web.DuckDuckGo.MaxResults,
DuckDuckGoEnabled: cfg.Tools.Web.DuckDuckGo.Enabled,

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

Copilot AI review requested due to automatic review settings March 2, 2026 16:17
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

Copy link
Collaborator

@yinwm yinwm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yinwm yinwm merged commit 0150947 into sipeed:main Mar 2, 2026
2 checks passed
@yinwm
Copy link
Collaborator

yinwm commented Mar 2, 2026

thanks for the pr, I am waiting for this for long time

@Orgmar
Copy link
Contributor

Orgmar commented Mar 4, 2026

@yuchou87 MCP集成这个PR工程量很大,stdio/SSE/HTTP三种传输协议都支持了,工具自动注册用 mcp_{server}_{tool} 的命名也很清晰。Docker双镜像策略也很实际,轻量版和带Node.js完整版各有各的场景。环境变量优先级处理(envFile < config.Env)和生命周期管理这些细节也考虑到了,1500多行代码加测试覆盖,质量很扎实。

对了,我们在Discord上建了 PicoClaw Dev Group,贡献者们可以在里面交流技术方案和协作开发。如果你感兴趣,发邮件到 [email protected],主题写 [Join PicoClaw Dev Group] + 你的GitHub账号,我们会发Discord邀请链接给你!

hyperwd pushed a commit to hyperwd/picoclaw that referenced this pull request Mar 5, 2026
Pluckypan pushed a commit to Pluckypan/picoclaw that referenced this pull request Mar 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants