fix(mcp): slots flag guard, error detail, and dropped tool params#894
Open
rohitg00 wants to merge 2 commits into
Open
fix(mcp): slots flag guard, error detail, and dropped tool params#894rohitg00 wants to merge 2 commits into
rohitg00 wants to merge 2 commits into
Conversation
Slot tools dispatched to mem::slot-* functions that are only registered when AGENTMEMORY_SLOTS is enabled, so disabled installs got an opaque Internal error instead of guidance. The MCP dispatch now returns the same structured error/flag/enableHow body the REST endpoints already use, and tools/list stops advertising slot tools while the flag is off. The dispatch catch-all now includes the underlying error message so real failures are diagnosable instead of a bare Internal error. memory_save dropped array-typed concepts and files (string-only split) and the standalone shim dropped the project parameter in validation, the proxied remember body, and the local fallback record. Both now plumb through. memory_sessions returned every session unbounded; it now accepts a limit (default 20, max 100) and returns the most recent sessions sorted by startedAt descending. Covered by test/mcp-tools-call.test.ts plus new cases in the surface default and standalone proxy suites.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR gates slot tools in the MCP surface and call path, adds optional ChangesMCP Tool Improvements
Sequence Diagram(s)sequenceDiagram
participant Client
participant ToolsRegistry
participant MCPServer
participant SDK
participant KV
Client->>ToolsRegistry: getVisibleTools()
ToolsRegistry->>ToolsRegistry: filter out SLOT_TOOL_NAMES if slots disabled
Client->>MCPServer: mcp::tools::call(name, args)
MCPServer->>MCPServer: if name in SLOT_TOOL_NAMES && !isSlotsEnabled() -> return isError
alt slot call allowed
MCPServer->>SDK: trigger appropriate mem::... function (or proxy)
SDK->>KV: read/write as needed (e.g., sessions, remember)
SDK-->>MCPServer: trigger response
MCPServer-->>Client: return tool output
else proxy/local memory_save
MCPServer->>KV: persist with optional project
MCPServer-->>Client: return saved response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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/server.ts`:
- Line 271: The limit calculation for memory_sessions uses const limit =
Math.min(asNumber(args.limit, 20) ?? 20, 100); which allows 0 or negative values
and redundantly uses ?? 20; update it to enforce a minimum of 1 and drop the
redundant fallback by using Math.max(1, Math.min(asNumber(args.limit, 20),
100))). Locate the usage in the memory_sessions handler where limit is declared
(reference symbol: asNumber and variable limit) and replace the expression so
limit is always between 1 and 100.
🪄 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: a09a0762-1d4f-4be4-bc24-7d67b6260c2e
📒 Files selected for processing (6)
src/mcp/server.tssrc/mcp/standalone.tssrc/mcp/tools-registry.tstest/mcp-standalone-proxy.test.tstest/mcp-surface-default.test.tstest/mcp-tools-call.test.ts
|
|
||
| case "memory_sessions": { | ||
| const sessions = await kv.list(KV.sessions); | ||
| const limit = Math.min(asNumber(args.limit, 20) ?? 20, 100); |
Contributor
There was a problem hiding this comment.
Enforce minimum limit of 1 for memory_sessions.
The limit calculation allows negative or zero values (e.g., -5 or 0), which would result in an empty array via slice(0, limit). Other tools like memory_smart_search (line 294) and memory_commits (line 1266) use Math.max(1, Math.min(...)) to enforce a minimum of 1.
Additionally, the ?? 20 is redundant since asNumber(args.limit, 20) already returns 20 when the value is not finite.
🛡️ Proposed fix
- const limit = Math.min(asNumber(args.limit, 20) ?? 20, 100);
+ const limit = Math.max(1, Math.min(asNumber(args.limit, 20) ?? 20, 100));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const limit = Math.min(asNumber(args.limit, 20) ?? 20, 100); | |
| const limit = Math.max(1, Math.min(asNumber(args.limit, 20) ?? 20, 100)); |
🤖 Prompt for 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.
In `@src/mcp/server.ts` at line 271, The limit calculation for memory_sessions
uses const limit = Math.min(asNumber(args.limit, 20) ?? 20, 100); which allows 0
or negative values and redundantly uses ?? 20; update it to enforce a minimum of
1 and drop the redundant fallback by using Math.max(1,
Math.min(asNumber(args.limit, 20), 100))). Locate the usage in the
memory_sessions handler where limit is declared (reference symbol: asNumber and
variable limit) and replace the expression so limit is always between 1 and 100.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #888.
Slot tools dispatched to mem::slot-* functions that are only registered when AGENTMEMORY_SLOTS is enabled, so disabled installs got an opaque Internal error instead of guidance. The MCP dispatch now returns the same structured error/flag/enableHow body the REST endpoints already use, and tools/list stops advertising slot tools while the flag is off, so clients no longer see tools that fail by construction.
Also in this change: the dispatch catch-all now includes the underlying error message; memory_save accepts array-typed concepts and files (was string-only split, arrays silently became empty); the standalone shim plumbs the project parameter through validation, the proxied remember body, and the local fallback record; memory_sessions takes a limit (default 20, max 100) and returns the most recent sessions sorted by startedAt descending instead of the full unbounded list.
Tested by test/mcp-tools-call.test.ts plus new cases in the surface default and standalone proxy suites.
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation