Skip to content

feat(mcp): validate and wrap async functions as code tools - #759

Open
vishal-bala wants to merge 6 commits into
mainfrom
feat/mcp-code-tools/02-code-tool-wrapper
Open

vishal-bala wants to merge 6 commits into
mainfrom
feat/mcp-code-tools/02-code-tool-wrapper

Conversation

@vishal-bala

@vishal-bala vishal-bala commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator

Motivation

A code tool is an async function whose first parameter is a ToolContext and whose other parameters are the arguments the model supplies. This PR turns such a function into a registered MCP tool, with the same rails a built-in gets, applied around the function rather than left to its author. It builds on the ToolContext from the previous PR. Users still cannot reach it, because the server registers nothing yet.

Changes

Checked where the tool is declared

build_code_tool_spec(fn, ...) validates a function and returns a frozen CodeToolSpec. It refuses:

  • a sync function, since ctx.index is async and a sync tool could not use it;
  • a first parameter that is not a positional ToolContext. The wrapper passes the context positionally, so a keyword-only one would pass a shape check and then fail on every call;
  • any other parameter that is unannotated, *args, **kwargs or positional-only. The annotations become the tool's input schema;
  • a parameter type that no input schema can describe, such as a plain class. Otherwise it would surface only at startup, after every binding had connected;
  • a missing description. The docstring is used unless description= is given, because the description is what the model selects tools by.

The name is the function's name with hyphens for underscores, or name=, prefixed by namespace= when given. It is checked by validate_tool_name, now factored out of profile validation, so profiles and code tools share one naming rule. Annotations are resolved with typing.get_type_hints, so a module using postponed annotations is checked against the real types.

Applied on every call

register_code_tool(server, spec, binding_id) registers a wrapper whose advertised signature is the function's minus the context. This follows how register_profile_tool narrows its signature, so FastMCP never offers the context to the model, and it rejects a call that tries to pass one. On each call the wrapper:

  1. checks the server's read scope, plus the write scope for a tool declared with writes=True, whose handle can both read and write, then the tool's own scope= on top. A tool can therefore be stricter than the built-ins, never looser;
  2. refuses a writing tool whose binding is now read-only;
  3. builds the ToolContext, giving a writing tool the writable handle;
  4. bounds the whole call by the binding's request_timeout_seconds, so the tool's own code cannot hold a request open indefinitely. A timeout returns a retryable backend_unavailable that names the tool;
  5. maps any other exception through map_exception, so a ValueError reaches the client as invalid_request and no traceback leaks. A raised RedisVLMCPError passes through unchanged.

Startup will also refuse a writing tool on a read-only binding (next PR). The per-call check in step 2 is defence in depth, as in upsert_records, so a write never depends on that one check.

Notes

The whole-call timeout and the per-call handle timeout use the same value, so the outer bound usually fires first. That is deliberate: the per-call bound protects the semaphore, and the outer bound protects the request.

A test runs a timed-out call against the real run_guarded. It checks that the call releases its concurrency slot and the active-request count, since a leak there would remove one slot per timeout until the server stopped answering.

The whole-call timeout can also cancel a handle call while it is still queued for a concurrency slot. run_guarded now acquires the slot explicitly and closes the Redis call it never started, which previously logged "coroutine was never awaited" once per such timeout.

Release Notes

An async function becomes a code tool only if it takes a positional ToolContext, annotates its other parameters and has a description; every call is then checked against the read scope, the write scope for a writing tool, and any per-tool scope=, bounded by the binding's request timeout, and mapped to the MCP error contract.

Next Steps

The next PR in the stack adds server.code_tool(...) and the startup checks that pair each tool with its binding.

Reviewer Brief

Start here: redisvl/mcp/tools/code.py:179, register_code_tool. Its wrapper applies the scopes, the read-only check, the whole-call timeout and the error mapping around every code tool.

Where it could fail

  • redisvl/mcp/tools/code.py:187: every call needs the read scope, a writing tool the write scope too, then the tool's own scope=; all three must run before the binding is resolved.
  • redisvl/mcp/server.py:251: run_guarded now acquires its slot explicitly, so every path after the acquire must release it exactly once, and a call cancelled while queued must close the coroutine it never started.
  • redisvl/mcp/server.py:251 (pre-existing): the admission and bookkeeping steps hold _request_state_lock without awaiting inside it; an await added there later would let a timeout skip the active-request decrement and hang shutdown.

Not verified: FastMCP 2.x; cluster mode.

Beyond CI: mutants removing the server-wide scope check, the read-scope check for writing tools, the unstarted-call close, fullmatch and the whole-call timeout each fail a unit test; 3,000 run_guarded calls under random cancellation left no leaked slot and no never-awaited warning.

Since the last brief: 971de0f refuses a scope= with surrounding whitespace, which could never match a token.

Reviewed 8b8dee6, fixes in f6f3c49 · Claude Code (claude-opus-5-5) · lenses: correctness, security, design, tests, docs, Redis behaviour


Note

Medium Risk
Changes request admission (run_guarded) affect all MCP tools, and new code-tool wrappers enforce auth scopes and timeouts on every call—important for security but with broad runtime impact.

Overview
Adds code tools: async Python functions with a positional ToolContext first argument can be validated via build_code_tool_spec and registered with register_code_tool, using the same safety rails as built-ins (scopes, read-only writes, binding timeout, MCP error mapping). The model-facing schema hides ToolContext and is built from annotated parameters.

Shared naming: validate_tool_name is extracted from profile config validation and uses fullmatch so names with trailing newlines (e.g. foo\n) are rejected; custom profile tools delegate to this helper.

Concurrency fix: run_guarded explicitly acquires/releases the semaphore so timeouts or cancellation while queued still release the slot and close coroutines that never started—preventing leaked concurrency capacity and “coroutine was never awaited” warnings.

Server startup does not register code tools yet (follow-up PR); this PR is the library + tests only.

Reviewed by Cursor Bugbot for commit ecf774a. Bugbot is set up for automated code reviews on this repo. Configure here.

@vishal-bala
vishal-bala added this pull request to stack #762 October 14, 2026 09:01
@vishal-bala vishal-bala added the auto:minor Increment the minor version when merged label Oct 15, 2026
@vishal-bala
vishal-bala marked this pull request as ready for review October 27, 2026 13:47
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-27T13:51:03.222289Z 26130ee Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@vishal-bala
vishal-bala requested a review from limjoobin October 27, 2026 13:47

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 26130eefa8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread redisvl/mcp/config.py Outdated
tool may be called. ``kind`` names the source in the message, so an error
points at the config section or decorator that produced the name.
"""
if not _TOOL_NAME_PATTERN.match(name):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject tool names with a trailing newline

When an explicit code-tool name or namespace ends in \n, Python's $ anchor matches immediately before that final newline, so _TOOL_NAME_PATTERN.match(name) accepts a value that contradicts the stated character set. The invalid name is then passed unchanged to FastMCP and may be rejected by MCP clients or exposed as an unusable tool; use a full-string match such as fullmatch() (or \Z) instead.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 9c4bf86: validate_tool_name now uses fullmatch, so a name or namespace ending in a newline is refused. This covers profiles and code tools, which share the check; regression cases were added for both.

🤖 Addressed by Claude Code

@limjoobin limjoobin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Found an issue with permission-scoping that should be resolved before merging!

Comment thread redisvl/mcp/tools/code.py

@limjoobin limjoobin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks! The permission-scoping issue has been addressed with appropriate regression coverage.

@cursor cursor Bot 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.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 8b8dee6. Configure here.

Comment thread redisvl/mcp/tools/code.py
Base automatically changed from feat/mcp-code-tools/01-tool-context to main October 30, 2026 13:44
build_code_tool_spec checks a code tool where it is declared: it must be
async, take a ToolContext first, annotate every other parameter, have a
description, and carry a name the profile rules accept (now shared through
validate_tool_name). register_code_tool hides the context from the advertised
schema, checks the read or write scope and any per-tool scope before the
function runs, bounds the whole call by the binding's request timeout, and
maps exceptions to the MCP error contract.
A ToolContext declared keyword-only passed validation and then failed every
call, because the wrapper passes it positionally; it is now refused where it
is declared. A writing tool re-checks that its binding is still writable on
each call, since tools register once while a restart re-reads the config.
Name errors now point at the function, and the wrapper's tests cover the
binding it resolves, a call over the wire and a timed-out call releasing its
concurrency slot.
@vishal-bala
vishal-bala force-pushed the feat/mcp-code-tools/02-code-tool-wrapper branch from 971de0f to ecf774a Compare October 30, 2026 13:44

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto:minor Increment the minor version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants