Repository navigation
feat(mcp): validate and wrap async functions as code tools - #759
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 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".
limjoobin
left a comment
There was a problem hiding this comment.
Found an issue with permission-scoping that should be resolved before merging!
limjoobin
left a comment
There was a problem hiding this comment.
LGTM, thanks! The permission-scoping issue has been addressed with appropriate regression coverage.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ 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.
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.
…close a queued call on timeout
971de0f to
ecf774a
Compare
## Motivation
`server.code_tool(...)` from the previous PR only helps when the
caller's own code constructs the server. The common deployment is `rvl
mcp --config`, which imports no user code. This PR lets the config name
modules for the server to import. Because importing a module runs its
code with the server's privileges, nothing is imported unless the
operator opts in separately.
## Changes
`@code_tool(...)`, exported from `redisvl.mcp`, takes the same arguments
as `server.code_tool`. It checks the function and attaches its spec as
an attribute, without registering it anywhere:
```python
# myapp/support_tools.py
from redisvl.mcp import ToolContext, code_tool
@code_tool(index="tickets", namespace="support")
async def count_open_tickets(ctx: ToolContext, product: str) -> dict:
"""Count open support tickets for a product."""
...
```
```yaml
server:
redis_url: redis://localhost:6379
allow_code_tools: true
code_tool_modules: [myapp.support_tools]
```
At startup, the server imports each listed module before any binding
connects. Every function marked with `@code_tool` is validated and
registered alongside tools from `server.code_tool`, under the same
startup checks.
Startup refuses, naming the module or setting:
| Refused when | Why |
|---|---|
| `code_tool_modules` is set without `allow_code_tools: true` (default
`false`) | The import runs operator code; the opt-in keeps a
profiles-only config from running any Python. Checked at config load. |
| A module entry is blank, padded or listed twice | Almost certainly a
typo. |
| A module cannot be imported, or raises while importing | Reported by
module name, with the cause chained. |
| A module defines no `@code_tool` function | A misspelled module name
should not quietly produce a server with fewer tools. |
| A decorator is applied over `@code_tool` | `functools.wraps` copies
the marker while the spec still calls the inner function, so the outer
decorator would silently never run. |
The marker lives on the function, not in a module-level registry.
Importing a module therefore registers nothing anywhere, and two servers
in one process stay independent, as do tests that import a tool module.
The loader considers only functions and reads the marker from each
function's own `__dict__`, so a module attribute with a permissive
`__getattr__`, such as a lazy module proxy, is never consulted. A tool
one listed module re-exports from another registers once.
The docs gain "Code Tools Loaded From Modules" and a troubleshooting
entry. They state that `rvl` does not put the working directory on the
import path: modules must be installed or on `PYTHONPATH`, and under
`uvx` installed in the environment it builds.
## Notes
The loader does not edit `sys.path`. Adding the config's directory would
make import resolution depend on where the config file happens to live.
I verified the end-to-end flow manually with `rvl mcp --transport
streamable-http`, a scratch module on `PYTHONPATH` and the server
started from outside the project directory. A FastMCP client listed
`support-count-open-tickets` with only `product` in its schema, and a
call returned the expected count.
## Release Notes
`rvl mcp` can load code tools from Python modules. Mark async functions
with `@code_tool` from `redisvl.mcp`, list their modules under
`server.code_tool_modules`, and set `server.allow_code_tools: true`,
which defaults to `false` because importing a module runs its code with
the server's privileges. Modules must be importable by the server
process.
## Next Steps
- Update the custom-tools proposal to record where v1.1 departs from it:
the `code_tool` names, no `ctx.guarded` (the handle applies the limits
itself), id-only writes and no filter-wide bulk writes, no code tools on
tenant-scoped indexes, async tools only, and the whole-call timeout.
## Reviewer Brief
**Start here:** `redisvl/mcp/tools/code.py:269`, `load_code_tools`. It
imports the modules a config names, so it is where configuration becomes
running code.
**Where it could fail**
- `redisvl/mcp/config.py:266`: `allow_code_tools` is the only gate on
importing operator code; check no settings or environment path can set
`code_tool_modules` around it.
- `redisvl/mcp/tools/code.py:269`: the loader reads the `@code_tool`
marker from each function's own `__dict__` and refuses a decorator
wrapped over it; check a tool re-exported from a second module registers
once.
**Not verified:** importing modules under `uvx`, which the guide's
advice rests on reasoning about how `uvx` builds its environment;
FastMCP 2.x.
**Since the last brief:** `927124e` rebases the PR onto `main` as one
commit after #758, #759 and #760 were squash-merged; the code is
unchanged from the reviewed head plus its fixes.
<sub>Reviewed `6ff481b`, fixes in `cc9249f` · Claude Code
(claude-opus-5-5) · lenses: correctness, security, design, tests, docs,
Redis behaviour</sub>
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Medium Risk**
> When **`allow_code_tools`** is enabled, listed modules run with the
MCP server process privileges at import time; misconfiguration or
untrusted modules are the main exposure, though defaults stay off and
config validation fails closed on bad entries.
>
> **Overview**
> **`rvl mcp --config`** can now register custom code tools without
constructing **`RedisVLMCPServer`** in Python. Add **`@code_tool`**
(exported from **`redisvl.mcp`**, same options as
**`server.code_tool`**) on async functions; list modules under
**`server.code_tool_modules`** and set **`server.allow_code_tools:
true`** (defaults **`false`**).
>
> At startup—**before** any Redis binding connects—the server imports
each listed module via **`load_code_tools`**, collects functions marked
with **`__redisvl_mcp_tool__`**, and registers them through the same
validation path as tools added with **`server.code_tool`** (name
collisions, index binding, read-only/write rules, tenant
**`lock.inject`** routes). Config load rejects **`code_tool_modules`**
without the opt-in, duplicate or blank module names, failed imports,
modules with no **`@code_tool`** functions, and decorators wrapped
outside **`@code_tool`**.
>
> Docs add module-based registration, **`PYTHONPATH`** / **`uvx`**
import notes, and troubleshooting for module load failures.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
927124e. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

Motivation
A code tool is an async function whose first parameter is a
ToolContextand 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 theToolContextfrom 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 frozenCodeToolSpec. It refuses:ctx.indexis async and a sync tool could not use it;ToolContext. The wrapper passes the context positionally, so a keyword-only one would pass a shape check and then fail on every call;*args,**kwargsor positional-only. The annotations become the tool's input schema;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 bynamespace=when given. It is checked byvalidate_tool_name, now factored out of profile validation, so profiles and code tools share one naming rule. Annotations are resolved withtyping.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 howregister_profile_toolnarrows 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:writes=True, whose handle can both read and write, then the tool's ownscope=on top. A tool can therefore be stricter than the built-ins, never looser;ToolContext, giving a writing tool the writable handle;request_timeout_seconds, so the tool's own code cannot hold a request open indefinitely. A timeout returns a retryablebackend_unavailablethat names the tool;map_exception, so aValueErrorreaches the client asinvalid_requestand no traceback leaks. A raisedRedisVLMCPErrorpasses 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_guardednow 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-toolscope=, 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 ownscope=; all three must run before the binding is resolved.redisvl/mcp/server.py:251:run_guardednow 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_lockwithout awaiting inside it; anawaitadded 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,
fullmatchand the whole-call timeout each fail a unit test; 3,000run_guardedcalls under random cancellation left no leaked slot and no never-awaited warning.Since the last brief:
971de0frefuses ascope=with surrounding whitespace, which could never match a token.Reviewed
8b8dee6, fixes inf6f3c49· Claude Code (claude-opus-5-5) · lenses: correctness, security, design, tests, docs, Redis behaviourNote
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
ToolContextfirst argument can be validated viabuild_code_tool_specand registered withregister_code_tool, using the same safety rails as built-ins (scopes, read-only writes, binding timeout, MCP error mapping). The model-facing schema hidesToolContextand is built from annotated parameters.Shared naming:
validate_tool_nameis extracted from profile config validation and usesfullmatchso names with trailing newlines (e.g.foo\n) are rejected; custom profile tools delegate to this helper.Concurrency fix:
run_guardedexplicitly 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.