Skip to content

fix(mcp): proxy all 51 tools to server, not just 7 hardcoded IMPLEMENTED_TOOLS (closes #234)#238

Closed
cl0ckt0wer wants to merge 2 commits intorohitg00:mainfrom
cl0ckt0wer:fix/234-mcp-tools-all
Closed

fix(mcp): proxy all 51 tools to server, not just 7 hardcoded IMPLEMENTED_TOOLS (closes #234)#238
cl0ckt0wer wants to merge 2 commits intorohitg00:mainfrom
cl0ckt0wer:fix/234-mcp-tools-all

Conversation

@cl0ckt0wer
Copy link
Copy Markdown

@cl0ckt0wer cl0ckt0wer commented May 8, 2026

Summary

Fixes #234agentmemory mcp --tools all now returns all 51 tools instead of silently filtering to 7.

Root cause

src/mcp/standalone.ts had a hardcoded IMPLEMENTED_TOOLS set of 7 tool names. Both tools/list and validate() applied this filter unconditionally, even when the server was reachable in proxy mode. getVisibleTools() correctly returned all 51 tools when AGENTMEMORY_TOOLS=all was set, but the filter then cut them down to 7.

Changes

src/mcp/standalone.ts

  1. tools/list now proxy-aware: When in proxy mode, fetches all tools from GET /agentmemory/mcp/tools (server returns unfiltered 51). Falls back to the local filtered list if the server is unreachable or returns an unexpected shape.

  2. handleToolCall proxies ALL tools: Proxy resolution now happens before validation. In proxy mode, any tool call is forwarded to POST /agentmemory/mcp/call with { name, arguments } payload — the server's mcp::tools::call endpoint already has handlers for all 51 tools.

  3. Removed handleProxy(): The 7-tool switch that mapped each tool to a different REST endpoint is replaced by the generic /agentmemory/mcp/call endpoint. This eliminated ~54 lines of per-tool proxy code.

  4. Local mode preserved: In local mode (no server), IMPLEMENTED_TOOLS filter and validate() are preserved — the InMemoryKV genuinely only supports 7 operations.

test/mcp-standalone.test.ts

  • Mocked resolveHandle to return local mode for all local tests (prevents server-probe side effects)
  • Added 4 tests: proxy mode forwards non-IMPLEMENTED_TOOLS, sends correct payload, local mode still rejects unknown tools, local mode IMPLEMENTED_TOOLS still work

test/mcp-standalone-proxy.test.ts

  • Updated proxy tests to verify the new generic /agentmemory/mcp/call endpoint
  • Verified proxy mode delegates validation to server (empty content arg passes through)
  • Verified auth headers attach to probe AND /agentmemory/mcp/call

Verification

# Before: only 7 tools
$ npx @agentmemory/agentmemory mcp --tools all
# tools/list returns 7 tools (memory_save, memory_recall, memory_smart_search,
#   memory_sessions, memory_export, memory_audit, memory_governance_delete)

# After: all 51 tools
$ npx @agentmemory/agentmemory mcp --tools all  
# tools/list returns 51 tools (all MCP tools including slot_list, lesson_save,
#   crystallize, reflect, signal_send, action_create, etc.)

Non-IMPLEMENTED_TOOLS calls now succeed:

$ curl -X POST http://localhost:3111/agentmemory/mcp/call \
  -d '{"name":"memory_lesson_save","arguments":{"content":"test"}}'
# Returns: {"content":[{"text":"{\n  \"action\": \"created\"..."}]}
# (memory_lesson_save was not in the 7-tool IMPLEMENTED_TOOLS set)

Test results

  • 35/35 tests pass (28 standalone + 7 proxy)
  • Full suite: 810/818 pass (8 pre-existing failures unrelated to this change)

Summary by CodeRabbit

  • New Features

    • Added a unified proxy-based tool invocation path that forwards tool requests to remote MCP proxies while falling back to local execution when needed.
    • Extended tools listing to optionally retrieve available tools from a remote proxy.
  • Bug Fixes

    • Improved proxy failure handling and response-shape validation; ensures sensible fallback and clearer proxy-error reporting.
    • Ensures authentication token is forwarded with proxied requests.
  • Tests

    • Expanded test coverage for proxy mode, delegation, auth forwarding, and failure/retry behavior.

…_TOOLS (closes rohitg00#234)

- tools/list now fetches from GET /agentmemory/mcp/tools in proxy mode, returning all 51 tools instead of filtering to 7
- handleToolCall proxies ALL tools to POST /agentmemory/mcp/call in proxy mode, skipping local IMPLEMENTED_TOOLS validation
- Removed handleProxy() — the generic /agentmemory/mcp/call endpoint replaces per-tool REST endpoint mapping
- In local mode (no server), IMPLEMENTED_TOOLS filter and validate() are preserved for InMemoryKV compatibility
- Added 4 tests for proxy mode forwarding, payload shape, and local-mode behavior
@vercel
Copy link
Copy Markdown

vercel Bot commented May 8, 2026

@cl0ckt0wer is attempting to deploy a commit to the rohitg00's projects Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 8, 2026

Review Change Stack
No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 00548aa9-25ce-4c57-bd27-ef552e6b171f

📥 Commits

Reviewing files that changed from the base of the PR and between f38104b and d897ee1.

📒 Files selected for processing (2)
  • src/mcp/standalone.ts
  • test/mcp-standalone.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • test/mcp-standalone.test.ts
  • src/mcp/standalone.ts

📝 Walkthrough

Walkthrough

The PR unifies MCP proxy dispatch: tool calls resolve a handle and, in proxy mode, are POSTed to /agentmemory/mcp/call (with array-shaped content returned directly); proxy failures invalidate the handle and fall back to local execution. tools/list delegates to /agentmemory/mcp/tools with local fallback. Tests expanded accordingly.

Changes

MCP Proxy Tool Dispatch Unification

Layer / File(s) Summary
Proxy Integration in Tool Call Handler
src/mcp/standalone.ts
handleToolCall now resolves a mode handle before validation, routes proxied calls to POST /agentmemory/mcp/call with { name, arguments }, returns array-shaped proxy content directly (otherwise wraps as text), invalidates handle on proxy failure, rethrows for non-implemented tools, and falls back to local KV for implemented tools. tools/list attempts /agentmemory/mcp/tools in proxy mode and falls back to local filtered getVisibleTools() on errors or unexpected shapes.
Proxy Behavior Tests
test/mcp-standalone-proxy.test.ts
Integration tests verify proxied dispatch for implemented and non-implemented tools, POST payload correctness, bearer auth propagation to both probe and proxied calls, server-provided responses used without local validation, probe/re-probe behavior, and local fallback response shapes on connection refusal.
Local and Proxy Mode Unit Tests
test/mcp-standalone.test.ts
Unit tests mock rest-proxy.js to control resolveHandle mode; they assert proxy mode forwards unimplemented tools and parses remote responses, validate POST /agentmemory/mcp/call request bodies, confirm local-mode unknown-tool errors and local execution for implemented tools, and ensure proxy call failure messages are propagated with a proxy call failed for <toolName>: prefix.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

  • rohitg00/agentmemory#161: Modifies src/mcp/standalone.ts to add proxy-aware handling and local fallback; related proxy dispatch changes.
  • rohitg00/agentmemory#141: Changes to src/mcp/standalone.ts and tests around MCP tools and validation helpers; overlaps with testing and dispatch logic.

Poem

🐇 I hopped through handlers, keen and spry,

Routed calls to servers in the sky,
When proxy stumbles, I nudge local ground,
Both lists and calls now follow sound,
A little hop, a test—then joy abound.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing MCP to proxy all 51 tools to the server instead of filtering to 7 hardcoded tools, and references the linked issue #234.
Linked Issues check ✅ Passed The PR implements the core requirements: tools/list delegates to server in proxy mode via GET /agentmemory/mcp/tools, handleToolCall proxies all tool calls via POST /agentmemory/mcp/call with correct payload, and local fallback behavior is preserved.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing issue #234: proxying tools through the server, updating tool call handling, and adding corresponding test coverage.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/mcp/standalone.ts`:
- Around line 259-268: The proxy catch block currently only logs the proxy error
and always falls through to validate() causing an "Unknown tool" error for tools
that aren't locally implemented; change the behavior so that inside the catch
you check whether toolName is in IMPLEMENTED_TOOLS (or equivalent set), and if
it is not, rethrow or throw a new Error that includes the original proxy error
message (preserving err) so callers see the connectivity/server error; if the
tool is implemented locally, keep calling invalidateHandle() and fall back to
validate(toolName, args) and handleLocal(validated, kvInstance).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3094ecd2-c5f7-468b-b9e1-827c6370ec89

📥 Commits

Reviewing files that changed from the base of the PR and between 1effa2c and f38104b.

📒 Files selected for processing (3)
  • src/mcp/standalone.ts
  • test/mcp-standalone-proxy.test.ts
  • test/mcp-standalone.test.ts

Comment thread src/mcp/standalone.ts
@cl0ckt0wer
Copy link
Copy Markdown
Author

Fixed the CodeRabbit review finding in src/mcp/standalone.ts:259-268:

Problem: The proxy catch block only logged the error and always fell through to validate(), which threw a misleading "Unknown tool" error for tools that aren't locally implemented — hiding the real connectivity/server error from the caller.

Fix: Inside the catch block, check IMPLEMENTED_TOOLS.has(toolName):

  • If the tool is not locally implemented → rethrow a new Error that includes the original proxy error message (e.g. "proxy call failed for memory_crystallize: ECONNREFUSED")
  • If the tool is locally implemented → log and fall back to local KV as before

Added a test in test/mcp-standalone.test.ts: "in proxy mode, non-IMPLEMENTED_TOOLS tool proxy failure rethrows with proxy error" — verifies that memory_crystallize (not in IMPLEMENTED_TOOLS) in proxy mode throws the proxy error on failure instead of "Unknown tool".

@rohitg00
Copy link
Copy Markdown
Owner

rohitg00 commented May 8, 2026

Reviewed — this is a clean fix for a real regression. The hardcoded IMPLEMENTED_TOOLS filter inside validate() and tools/list was directly contradicting getVisibleTools() returning all 51 when AGENTMEMORY_TOOLS=all is set.

Looks good:

  • Proxy-aware tools/list falls back to local list on proxy failure (good defensive default).
  • New test in mcp-standalone-proxy.test.ts covers the /mcp/call proxy path including auth-header passthrough.
  • validate() no longer rejects extended-toolset names — agreed; that filter belonged at the visibility layer, not validation.

Approving for merge. Holding the actual merge button until @rohitg00 confirms — but no blockers from my side.

(Worth flagging in the CHANGELOG under Fixed: "MCP standalone proxy now exposes all 51 tools when AGENTMEMORY_TOOLS=all, not just the 7 hardcoded ones (#234)".)

@rohitg00
Copy link
Copy Markdown
Owner

Thanks @cl0ckt0wer — root cause analysis here was exactly right, and your tests for proxy/auth/fallback shaped the landed fix.

Superseded by #270 (merged in v0.9.6, 2026-05-10), which implements the same approach: probe livez, proxy tools/list + tools/call to the server when reachable, fall back to local 7-tool IMPLEMENTED_TOOLS only when no server. Closing #234 once we confirm with @jcalfee on the remaining Docker-image report.

Closing as superseded — not because the work wasn't valuable. Appreciate the deep diagnostic on the original issue.

@rohitg00 rohitg00 closed this May 10, 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.

MCP shim: --tools all returns only 7 tools due to IMPLEMENTED_TOOLS hardcode

2 participants