fix: ignore unexpected MCP tool args during handler dispatch by apajon · Pull Request #572 · MemPalace/mempalace

apajon · 2026-04-10T20:57:29Z

Summary

This PR makes the MCP dispatcher more defensive by filtering incoming tool
arguments against the target Python handler signature before dispatch.

Today, the dispatcher forwards all client-provided arguments directly to the
handler. If the client sends an extra or mismatched parameter, the call fails
with a Python TypeError, which is surfaced to the MCP client as
-32000 Internal tool error. :contentReference[oaicite:0]{index=0}

This patch keeps valid behavior unchanged while preventing unexpected extra
arguments from crashing tool execution. :contentReference[oaicite:1]{index=1}

Problem

The current dispatch path effectively does this:

handler(**tool_args)

That means any client-side mismatch immediately becomes a runtime failure if
the handler does not accept one of the provided keywords. According to the
reproduction already collected, this is the root cause of several MCP tool
failures returning -32000 Internal tool error.

Reproduction

A concrete reproduced case is mempalace_search.

Client request arguments:

{
  "query": "test",
  "wing": "flexweld",
  "room": "architecture",
  "top_k": 3
}

Observed handler signature:

mempalace_search(query, limit, wing, room)

Because top_k is not accepted by the handler, dispatch fails with:

TypeError: got an unexpected keyword argument 'top_k'

and the MCP client receives:

-32000 Internal tool error

The same mismatch pattern was also observed on other tools where the client
sends extra arguments not accepted by the underlying handler, including:

mempalace_check_duplicate
mempalace_kg_add
mempalace_traverse

Root cause

There is a mismatch between the arguments sent by the MCP client and the
actual Python handler signatures.

The dispatcher does not currently filter or normalize arguments before calling
the handler, so unexpected keywords propagate directly into Python function
calls and raise TypeError.

Fix

Filter incoming arguments using the target handler signature before dispatch.

Conceptually:

import inspect

handler = TOOLS[tool_name]["handler"]
valid_params = set(inspect.signature(handler).parameters.keys())
filtered_args = {k: v for k, v in tool_args.items() if k in valid_params}
result = handler(**filtered_args)

This makes dispatch resilient to extra client-side arguments while preserving
normal behavior for all valid parameters.

Validation

I confirmed the issue by reproducing a direct tools/call request path for
mempalace_search using top_k, which fails under the current dispatcher. I
also confirmed that the same flow succeeds when either:

top_k is replaced with the correct handler parameter limit, or
unexpected arguments are filtered before handler dispatch.

This indicates the failure is in the MCP dispatch layer rather than in
ChromaDB or the underlying search implementation.

Scope

This PR is intentionally small and defensive.

It does not:

refactor tool schemas
rename parameters globally
change handler signatures
modify search, embedding, or database behavior

It only prevents unexpected extra arguments from crashing the dispatcher.

Future work

Possible follow-up improvements, outside the scope of this PR:

align tool schemas and handler signatures more strictly
normalize aliases such as top_k -> limit
add stricter schema-based validation or clearer client-facing errors

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

web3guru888

This is a clean, well-scoped fix for a real pain point.

The problem is real. We hit this exact failure pattern in our integration — MCP clients (Claude Desktop, OpenCode, and a few custom ones) routinely send extra fields like top_k, limit, format, or verbose that don't match the handler signatures. The -32000 surface makes debugging miserable because it gives no indication of which argument was unexpected.

The fix is correct. Using inspect.signature(handler).parameters to filter before dispatch is exactly the right approach: it's standard library, zero overhead for typical tool call volume, and it doesn't touch handler logic or tool schemas.

One thing worth noting: inspect.signature() follows __wrapped__ on decorated functions, which means if any handlers later get functools.wraps-decorated, the filtering will still work correctly on the underlying signature. That's a nice property to have implicitly.

The test is good. test_mcp_dispatch_ignores_unexpected_tool_arguments covers the core case cleanly. One additional edge case worth considering in a follow-up: a handler that accepts **kwargs (none currently, but possible in future) — in that case valid_params won't see the kwargs param, and the filter would incorrectly strip keys the handler would have accepted. The fix for that would be to also check inspect.Parameter.VAR_KEYWORD in the param kinds before filtering. Not a blocker for this PR since no current handlers use **kwargs.

Interaction with #571 — the object → obj rename in that PR adds dispatcher-level remapping for the external JSON key. These two PRs touch handle_request in adjacent spots and should be reviewed together to confirm they compose cleanly.

LGTM. The scope is right — fix dispatch defensiveness now, align schemas separately.

…compress keys - Default new collections to cosine distance (hnsw:space=cosine) instead of L2 — fixes negative similarity scores (MemPalace#568) - Filter unexpected MCP tool args before dispatch — prevents TypeError crash from extra params like top_k (MemPalace#572) - Rotate WAL at 10 MB with one backup to prevent unbounded growth (MemPalace#573) - Fix cmd_compress KeyError: align dict keys with compression_stats() return values (MemPalace#569) - Fix spellcheck _load_known_names: read from "people" key, use dict keys as canonical names (MemPalace#570) - Repair command uses cosine distance for rebuilt collection Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

jphein · 2026-04-10T21:52:11Z

Incorporated into #562 — filtering tool_args to schema_props.keys() before dispatch. Clean fix.

web3guru888

This is a real problem — the top_k → limit mismatch has come up in multiple issues (e.g. #538, #554). MCP clients should not be able to crash the server with a mismatched parameter name.

The inspect.signature() approach to filter args before dispatch is correct. A few implementation notes:

Keyword-only args: sig.parameters returns all parameters in order, including keyword-only ones (after *args or *). The filter should handle the case where a handler uses *args — in that case sig.parameters has a VAR_POSITIONAL kind param and the handler accepts arbitrary positional args. Worth checking that the filter doesn't silently drop valid args in that case (unlikely in MCP tools, but defensive).

Unknown arg logging: If the filter drops an unexpected arg like top_k, logging that at DEBUG level would help MCP client developers realize their schema is out of sync. A silent drop works but is harder to debug — the tool succeeds but ignores an expected parameter silently.

Alias mapping: The root fix would be to add top_k as an alias for limit in the search tool schema, so clients using either name get the right behavior. The arg filter is the right defensive layer, but an alias map in the MCP tool definition would prevent silent misses for intentional client-side parameter names. Not required for this PR but worth noting.

The test for mempalace_search with top_k as the concrete reproduction case is exactly what's needed. Good to have a direct regression test for the case that triggered the issue.

[MemPalace-AGI integration — 215 tests, 710 KG entities]

apajon · 2026-04-11T00:30:47Z

Thanks for the detailed review — really helpful.

Good point on the **kwargs case. That’s not something currently used in handlers, but it makes sense as a future-proofing concern. I’d keep this PR focused on filtering based on explicit parameters, and we can extend it in a follow-up if handlers start using VAR_KEYWORD.

Same for *args / VAR_POSITIONAL — not something expected in MCP tools, but worth keeping in mind for a more defensive implementation later.

The idea of adding debug logging for dropped arguments makes a lot of sense. I agree that silent drops can make client-side debugging harder. I’d prefer to keep this PR minimal and consider adding DEBUG-level logging in a separate change.

Alias mapping (e.g. top_k → limit) also seems like the right long-term fix at the schema level. This PR is more about making dispatch robust to mismatches, but aligning schemas would definitely improve usability.

I’ll double-check the interaction with #571 to make sure the dispatch changes compose cleanly.

Thanks again — this helped clarify the edge cases and future improvements.

jphein · 2026-04-11T00:32:50Z

Makes sense @apajon — keeping the PR focused on the explicit-param filtering is the right call. The debug logging and alias mapping are good follow-up items. Let us know if you need help testing the interaction with #571.

apajon · 2026-04-11T00:43:27Z

Thanks — that makes sense.

I’ll keep this PR focused as is, and follow up separately on logging and alias mapping.

I took a look at the interaction with #571, and there is one ordering concern to watch for.

If object -> obj remapping happens before the signature-based filtering, the two changes compose cleanly.

If filtering runs first, an incoming object key could be dropped as unexpected before it ever gets remapped to obj, which would break handlers expecting obj.

The safe order seems to be:

type coercion → object→obj remap → signature filtering → handler call

Not suggesting expanding this PR further — just flagging the interaction point since both changes touch handle_request.

web3guru888 · 2026-04-11T00:47:06Z

@apajon — the ordering analysis is exactly right, and it's a good call to document it even without expanding the PR.

The safe sequence you've described (type coercion → object→obj remap → signature filtering → handler call) is also the right order from a layered-abstraction perspective: each stage normalizes the input further before the next one sees it. Letting signature filtering run before the remap would make it order-sensitive in a fragile way — the filtering layer would need to know about schema-level aliases to avoid dropping them.

One thing worth noting: inspect.signature will correctly see obj (not object) after the remap in #571, so as long as the remap runs first, the filtering will pass obj through and drop the original object key naturally. No additional alias awareness needed in the filtering layer. The composition works cleanly if ordering is respected.

If #571 and this PR both land, a regression test that sends {"subject": ..., "predicate": ..., "object": ...} to mempalace_kg_add via tools/call would be the definitive proof that the full pipeline (remap + filter + handler) works end to end.

MemPalace-AGI integration — 208 discoveries, 215 tests | dashboard

apajon · 2026-04-11T00:52:10Z

That makes sense — thanks for the clarification.

The layered ordering you described matches what I had in mind as well, and it's reassuring that the filtering stays simple as long as the remap runs first.

The end-to-end test suggestion is a good idea. That would give a clear guarantee that the full pipeline (remap + filter + handler) behaves correctly. Happy to add that as a follow-up if it’s useful once both changes land.

…, MCP args, spellcheck, init docs - Remove overly broad \*[^*]+\* from EMOTION_MARKERS (MemPalace#536) - Replace Unicode checkmark with ASCII + for Windows cp1251/cp1252 (MemPalace#535) - Fix spellcheck reading from wrong entity registry key (MemPalace#570) - Add --yes flag to init instructions for non-interactive use (MemPalace#534) - Default KG query direction to 'both' instead of 'outgoing' - Fix KG path mismatch in MCP server (MemPalace#538) - Filter unexpected MCP tool args before dispatch (MemPalace#572) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

The schema-based argument filter (from MemPalace#647) strips all kwargs not declared in input_schema. This breaks handlers that accept **kwargs for pass-through to ChromaDB or other backends. Add inspect.Parameter.VAR_KEYWORD check before filtering — handlers with **kwargs receive all arguments unfiltered. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: skip arg whitelist for handlers accepting **kwargs (#572) The schema-based argument filter (from #647) strips all kwargs not declared in input_schema. This breaks handlers that accept **kwargs for pass-through to ChromaDB or other backends. Add inspect.Parameter.VAR_KEYWORD check before filtering — handlers with **kwargs receive all arguments unfiltered. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: guard inspect.signature failure, default to filtering Wrap inspect.signature() in try/except — on failure, default to filtering (safe fallback). Addresses Copilot feedback on fragility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

igorls · 2026-05-08T11:00:15Z

Hi, thanks for the contribution.

This PR has merge conflicts with develop, and the branch has not been updated in over 7 days, which puts it before our most recent release. The conflicts are likely against work that landed in that release.

Could you rebase onto develop so we can take another look?

If this change is no longer relevant, feel free to close the PR.

(This message is part of a periodic backlog pass, sent to all open PRs that match this state.)

fix: ignore unexpected MCP tool args during handler dispatch

8e818a3

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

apajon requested review from bensig and milla-jovovich as code owners April 10, 2026 20:57

web3guru888 approved these changes Apr 10, 2026

View reviewed changes

jphein mentioned this pull request Apr 10, 2026

feat: batch writes, concurrent mining, MCP tools, hooks, export, search improvements #562

Closed

11 tasks

web3guru888 reviewed Apr 10, 2026

View reviewed changes

This was referenced Apr 11, 2026

fix: standalone bug fixes (#534 #535 #536 #538 #570 #572 #637) #626

Closed

feat: MCP reliability — inode detection, WAL rotation, metadata cache #630

Closed

bensig changed the base branch from main to develop April 11, 2026 22:21

bensig requested a review from igorls as a code owner April 11, 2026 22:21

Copilot AI mentioned this pull request Apr 12, 2026

fix: skip arg whitelist for handlers accepting **kwargs (#572) #684

Merged

3 tasks

igorls added area/mcp MCP server and tools bug Something isn't working labels Apr 14, 2026

igorls added the needs-rebase PR has merge conflicts with develop and needs rebase label May 8, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ignore unexpected MCP tool args during handler dispatch#572

fix: ignore unexpected MCP tool args during handler dispatch#572
apajon wants to merge 1 commit into
MemPalace:developfrom
apajon:fix/mcp-ignore-unexpected-tool-args

apajon commented Apr 10, 2026

Uh oh!

web3guru888 left a comment

Uh oh!

jphein commented Apr 10, 2026

Uh oh!

web3guru888 left a comment

Uh oh!

apajon commented Apr 11, 2026

Uh oh!

jphein commented Apr 11, 2026

Uh oh!

apajon commented Apr 11, 2026

Uh oh!

web3guru888 commented Apr 11, 2026

Uh oh!

apajon commented Apr 11, 2026

Uh oh!

igorls commented May 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

apajon commented Apr 10, 2026

Summary

Problem

Reproduction

Root cause

Fix

Validation

Scope

Future work

Uh oh!

web3guru888 left a comment

Choose a reason for hiding this comment

Uh oh!

jphein commented Apr 10, 2026

Uh oh!

web3guru888 left a comment

Choose a reason for hiding this comment

Uh oh!

apajon commented Apr 11, 2026

Uh oh!

jphein commented Apr 11, 2026

Uh oh!

apajon commented Apr 11, 2026

Uh oh!

web3guru888 commented Apr 11, 2026

Uh oh!

apajon commented Apr 11, 2026

Uh oh!

igorls commented May 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants