Skip to content

fix(mcp_server): validate tool arguments before dispatch, return JSON-RPC errors on bad params - #91

Closed
christauff wants to merge 2 commits into
MemPalace:developfrom
christauff:fix/mcp-param-validation
Closed

fix(mcp_server): validate tool arguments before dispatch, return JSON-RPC errors on bad params#91
christauff wants to merge 2 commits into
MemPalace:developfrom
christauff:fix/mcp-param-validation

Conversation

@christauff

Copy link
Copy Markdown

Problem

handle_request() dispatches tool calls directly with **tool_args before any validation:

result = TOOLS[tool_name]["handler"](**tool_args)

If a caller passes wrong types or omits required parameters, raw Python exceptions leak into the JSON-RPC error response. Callers receive unstructured tracebacks instead of standards-compliant error objects. This also means any MCP client can probe internal implementation details through exception messages.

Fix

Add _validate_tool_args() — a lightweight validator that checks required parameters are present and that declared types match the tool's existing input_schema definitions:

def _validate_tool_args(tool_name: str, tool_args: dict, req_id) -> dict:
    schema = TOOLS[tool_name]["input_schema"]
    # check required params present
    # check declared types match
    # return JSON-RPC -32602 error dict on violation, or None if valid

Wire it into the tools/call handler before the dispatch call:

validation_error = _validate_tool_args(tool_name, tool_args, req_id)
if validation_error:
    return validation_error
result = TOOLS[tool_name]["handler"](**tool_args)

On violation, callers receive a proper {"code": -32602, "message": "Missing required parameter: 'query'"} error. The handler is never invoked. No logic changes to existing tools.

Test plan

  • Call search tool without query parameter → JSON-RPC error -32602 Missing required parameter: 'query'
  • Call kg_add tool with confidence as string instead of number → JSON-RPC error -32602 Parameter 'confidence' must be number, got str
  • All 19 tools with valid parameters → behavior unchanged, handlers invoked normally
  • Malformed JSON body → existing error handling unchanged (validation only runs after successful parse)

🤖 Generated with Claude Code

Add _validate_tool_args() helper that checks required parameters are
present and types match the tool's input_schema before delegating to
the handler. Returns a JSON-RPC -32602 (Invalid params) error response
on violation rather than surfacing raw Python exceptions to callers.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
8 tests covering valid args, missing required, wrong type, optional
params, number type coercion, unknown params, and no-required tools.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
GoodOlClint added a commit to GoodOlClint/mempalace that referenced this pull request Apr 7, 2026
Adds _validate_tool_args() that checks required params and types
against input_schema, returning JSON-RPC -32602 errors instead of
Python tracebacks. Applied after type coercion from PR MemPalace#84.

Upstream: MemPalace#91

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@bensig

bensig commented Apr 7, 2026

Copy link
Copy Markdown
Contributor

Solid work on the validation + tests. CI lint is failing though — can you run ruff check . and ruff format . and push a fix?

@web3guru888 web3guru888 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔧 Review of #91fix(mcp_server): validate tool arguments before dispatch, return JSON-RPC errors on bad params

Scope: +123/−0 · 2 file(s) · touches core

  • ⚠️ mempalace/mcp_server.py (modified: +40/−0)
  • tests/test_mcp_server.py (added: +83/−0)

Technical Analysis

  • 🔌 MCP server dispatch changes — verify JSON-RPC compliance and backward compatibility

Issues

  • ⚠️ Touches mempalace/mcp_server.py — Core MCP server — maintainer guards this closely

Suggestions

  • Magic number(s) 32602 — consider extracting to named constant(s)

Strengths

  • ✅ Includes test coverage

🟡 Needs attention — touches guarded files and has items to address.


🏛️ Reviewed by MemPalace-AGI · Autonomous research system with perfect memory · Showcase: Truth Palace of Atlantis

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

bensig commented Apr 12, 2026

Copy link
Copy Markdown
Contributor

Closing — this is superseded by recently merged PRs to develop. Thank you for the contribution!

@bensig bensig closed this Apr 12, 2026
igorls pushed a commit that referenced this pull request Aug 2, 2026
… audit found

hnsw_capacity_status() (chroma.py) exists precisely to preflight the
#1222 SIGSEGV/pyo3-panic class before anything touches the HNSW
segment, but repo-wide it was wired into only 4 call sites while raw
count()/collection.count() is called at 20+ others -- a bare
except Exception around count() cannot catch a native crash, since
the process dies regardless of any Python try/except. This wires the
existing, already-tested probe into the 7 remaining call sites the
audit identified as CRITICAL:

- #89 palace.py::_enforce_embedder_identity -- the universal
  get_collection() chokepoint every tool passes through, previously
  guarded only by except Exception. Highest leverage: skips this
  bookkeeping-only check on divergence instead of risking count().
- #90 migrate.py::migrate -- routes straight to the same
  SQLite-extraction fallback the except branch already used, instead
  of ever reaching col.count() when diverged.
- #91 repair.py::scan_palace / prune_corrupt -- both abort with the
  existing from-sqlite recovery guidance instead of opening the
  collection.
- #10 repair.py::rebuild_index -- preflights divergence alongside its
  existing sqlite-integrity and poisoned-max-seq-id preflights, before
  opening the collection.
- #13 repair.py::rebuild_index never rebuilt or reported on the
  closets collection -- now warns when closets is still diverged
  after a drawers-only rebuild, pointing at --mode from-sqlite instead
  of letting 'Repair complete' stand unqualified.
- #92 dedup.py::get_source_groups -- takes an optional palace_path
  (threaded from both callers) to preflight before count(); omitted by
  existing tests, which keep their pre-existing behavior.
- #93 miner.py::status -- preflights before the ChromaDB-client
  fallback path (used when the direct sqlite read is unavailable).

7 new regression tests, each confirmed failing against the pre-fix
code (via git stash of the source files only) and passing after the
fix. One existing dedup.py test updated for the new palace_path kwarg
in its call-signature assertion. Full suite: 3154 passed, 1 unrelated
pre-existing flake (test_mcp_server.py peer-writer-lock module-global
state leaking across test files in full-suite ordering -- this diff
never touches mcp_server.py).
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.

4 participants